mirror of
https://github.com/protocolbuffers/protobuf
synced 2026-08-26 02:23:14 -04:00
7d7d6ab836
Fixes #29023.
`Map#[]=` and `Message.new(map_field: {...})` build the map key as a `upb_StringView`
aliasing a Ruby String, then convert the value before `upb_Map_Set` copies the key. The
value conversion allocates, so it can trigger GC inside that window.
The aliased String is frequently a **temporary**: `Convert_RubyToUpb` replaces the caller's
object when the key is a Symbol (via `to_s`) or a String not already tagged UTF-8 (via
`Convert_CheckStringUtf8`), and nothing references the result once it returns. When GC
collects it, the freed block goes straight back to the next `upb_Arena_Malloc`, which
memcpys the *value* into it — leaving a silently corrupted key holding unrelated heap bytes,
tagged UTF-8 while containing invalid UTF-8, which then propagates into `encode`/`to_json`.
## The fix
Pass the arena at both insertion sites, so the key is copied before anything can allocate.
The lookup paths (`Map_index`, `Map_has_key`, `Map_delete`) keep the `NULL` fast path — they
consume the key immediately with no allocation in between, which is exactly the precondition
`Convert_StringData`'s comment describes. I reworded that comment, since it read as though
the aliasing were unconditionally safe; it holds for three of its five callers and not for
the two that insert.
Cost is one arena allocation per insert for string-typed keys. Non-string keys don't reach
`Convert_StringData` at all.
## Trigger
Needs both:
- a key that is a **Symbol**, or a String not already tagged UTF-8 — `ASCII-8BIT` is the
common case for anything read from a socket, a file, `Marshal`, or `String#pack`; and
- a value whose conversion allocates (a Symbol, or a non-UTF-8 String).
Plain UTF-8 keys are unaffected, which is presumably why this has gone unnoticed.
## Verification
Reproduces under **ordinary GC**, no `GC.stress` required — one corrupted key across 150k
iterations (0/50k, 0/50k, 1/50k), versus 100/100 with stress. That second number is an
existence proof rather than a rate.
Added regression tests to `ruby/tests/gc_test.rb` covering string keys, Symbol keys, and the
map-field kwarg path. Verified red/green against the same tree:
| ext build | new tests |
|---|---|
| unpatched `main` | 3 tests, **3 failures** |
| with this change | 3 tests, 300 assertions, **0 failures** |
Full Ruby suite green with the change on ruby 4.0.6 / arm64-darwin — `basic.rb` (133 tests,
157,864 assertions), `basic_proto2.rb` (93), `repeated_field_test.rb` (40),
`encode_decode_test.rb`, `memory_test.rb`, `object_cache_test.rb`, `well_known_types_test.rb`,
`service_test.rb`, `oom_test.rb`, `multi_level_nesting_test.rb` — 0 failures, 0 errors.
Reported separately via the channel in `SECURITY.md`, since this is a memory-safety issue in
an OT0 repository.
Closes #29026
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/29026 from jeremy:ruby-map-key-use-after-free
|
||
|---|---|---|
| .. | ||
| basic.rb | ||
| basic_proto2.rb | ||
| basic_test.proto | ||
| basic_test_features.proto | ||
| basic_test_proto2.proto | ||
| BUILD.bazel | ||
| common_tests.rb | ||
| encode_decode_test.rb | ||
| gc_test.rb | ||
| generated_code.proto | ||
| generated_code_editions.proto | ||
| generated_code_proto2.proto | ||
| generated_code_proto2_test.rb | ||
| generated_code_test.rb | ||
| golden-ruby_ffi_bindings.txt | ||
| implementation.rb | ||
| memory_test.rb | ||
| multi_level_nesting_test.proto | ||
| multi_level_nesting_test.rb | ||
| object_cache_test.rb | ||
| oom_test.rb | ||
| repeated_field_test.proto | ||
| repeated_field_test.rb | ||
| ruby_version.rb | ||
| service_test.proto | ||
| service_test.rb | ||
| stress.proto | ||
| stress.rb | ||
| test_import.proto | ||
| test_import_proto2.proto | ||
| test_ruby_package.proto | ||
| test_ruby_package_proto2.proto | ||
| type_errors.rb | ||
| utf8.proto | ||
| utf8.rb | ||
| well_known_types_test.rb | ||