dep-protobuf/ruby/tests/gc_test.rb
Jeremy Daer 7d7d6ab836 Ruby: fix use-after-free of map keys aliasing a temporary String (#29026)
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 e11cc7dfe2
PiperOrigin-RevId: 961236703
2026-08-07 19:44:44 -07:00

160 lines
5.2 KiB
Ruby
Executable file

#!/usr/bin/ruby
#
# generated_code.rb is in the same directory as this test.
$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__)))
old_gc = GC.stress
GC.stress = 0x01 | 0x04
require 'generated_code_pb'
require 'generated_code_proto2_pb'
GC.stress = old_gc
require 'test/unit'
class GCTest < Test::Unit::TestCase
def get_msg_proto3
A::B::C::TestMessage.new(
:optional_int32 => 1,
:optional_int64 => 1,
:optional_uint32 => 1,
:optional_uint64 => 1,
:optional_bool => true,
:optional_double => 1.0,
:optional_float => 1.0,
:optional_string => "a",
:optional_bytes => "b",
:optional_enum => A::B::C::TestEnum::A,
:optional_msg => A::B::C::TestMessage.new(),
:repeated_int32 => [1],
:repeated_int64 => [1],
:repeated_uint32 => [1],
:repeated_uint64 => [1],
:repeated_bool => [true],
:repeated_double => [1.0],
:repeated_float => [1.0],
:repeated_string => ["a"],
:repeated_bytes => ["b"],
:repeated_enum => [A::B::C::TestEnum::A],
:repeated_msg => [A::B::C::TestMessage.new()],
:map_int32_string => {1 => "a"},
:map_int64_string => {1 => "a"},
:map_uint32_string => {1 => "a"},
:map_uint64_string => {1 => "a"},
:map_bool_string => {true => "a"},
:map_string_string => {"a" => "a"},
:map_string_msg => {"a" => A::B::C::TestMessage.new()},
:map_string_int32 => {"a" => 1},
:map_string_bool => {"a" => true},
)
end
def get_msg_proto2
A::B::Proto2::TestMessage.new(
:optional_int32 => 1,
:optional_int64 => 1,
:optional_uint32 => 1,
:optional_uint64 => 1,
:optional_bool => true,
:optional_double => 1.0,
:optional_float => 1.0,
:optional_string => "a",
:optional_bytes => "b",
:optional_enum => A::B::Proto2::TestEnum::A,
:optional_msg => A::B::Proto2::TestMessage.new(),
:repeated_int32 => [1],
:repeated_int64 => [1],
:repeated_uint32 => [1],
:repeated_uint64 => [1],
:repeated_bool => [true],
:repeated_double => [1.0],
:repeated_float => [1.0],
:repeated_string => ["a"],
:repeated_bytes => ["b"],
:repeated_enum => [A::B::Proto2::TestEnum::A],
:repeated_msg => [A::B::Proto2::TestMessage.new()],
:required_int32 => 1,
:required_int64 => 1,
:required_uint32 => 1,
:required_uint64 => 1,
:required_bool => true,
:required_double => 1.0,
:required_float => 1.0,
:required_string => "a",
:required_bytes => "b",
:required_enum => A::B::Proto2::TestEnum::A,
:required_msg => A::B::Proto2::TestMessage.new(),
)
end
def test_generated_msg
old_gc = GC.stress
GC.stress = 0x01 | 0x04
from = get_msg_proto3
data = A::B::C::TestMessage.encode(from)
to = A::B::C::TestMessage.decode(data)
# This doesn't work for proto2 on JRuby because there is a nested required message.
# A::B::Proto2::TestMessage has :required_msg which is of type:
# A::B::Proto2::TestMessage so there is no way to generate a valid
# message that doesn't exceed the depth limit
if !defined? JRUBY_VERSION
from = get_msg_proto2
data = A::B::Proto2::TestMessage.encode(from)
to = A::B::Proto2::TestMessage.decode(data)
end
GC.stress = old_gc
puts "passed"
end
# Regression test: the map key must be copied into the arena, not aliased.
#
# Convert_RubyToUpb returns a *temporary* String for a key that is a Symbol or
# is not already tagged UTF-8. Converting the value afterwards allocates, which
# can trigger GC and free that temporary before upb_Map_Set copies the key --
# leaving a silently corrupted key holding unrelated heap bytes.
def assert_map_keys_survive_gc(&builder)
old_gc = GC.stress
GC.stress = true
begin
100.times do
# Non-UTF-8 key and value: the key conversion allocates a temporary, and
# the value conversion allocates again, opening the window.
key = ("K" * 5000).dup.force_encoding("ISO-8859-1") +
"\xE9".dup.force_encoding("ISO-8859-1")
value = ("V" * 5000).dup.force_encoding("ISO-8859-1") +
"\xE9".dup.force_encoding("ISO-8859-1")
assert_equal [key.encode("UTF-8")], builder.call(key, value).keys
end
ensure
GC.stress = old_gc
end
end
def test_map_string_key_not_corrupted_by_gc
assert_map_keys_survive_gc do |key, value|
map = Google::Protobuf::Map.new(:string, :string)
map[key] = value
map
end
end
def test_map_symbol_key_not_corrupted_by_gc
old_gc = GC.stress
GC.stress = true
begin
100.times do
map = Google::Protobuf::Map.new(:string, :string)
map[:some_symbol_key] = :some_symbol_value
assert_equal ["some_symbol_key"], map.keys
end
ensure
GC.stress = old_gc
end
end
def test_map_field_kwarg_key_not_corrupted_by_gc
assert_map_keys_survive_gc do |key, value|
A::B::C::TestMessage.new(:map_string_string => { key => value })
.map_string_string
end
end
end