Ruby: fix use-after-free of map keys aliasing a temporary String - #29026
Open
jeremy wants to merge 1 commit into
Open
Ruby: fix use-after-free of map keys aliasing a temporary String#29026jeremy wants to merge 1 commit into
jeremy wants to merge 1 commit into
Conversation
Map#[]= and Message.new(map_field: {...}) built the map key as a
upb_StringView aliasing a Ruby String, then converted the value before
upb_Map_Set copied the key. The value conversion allocates, so it can
trigger GC inside that window.
The aliased String is often 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 Convert_RubyToUpb returns. When GC collects
it, the freed block is handed straight back to the next
upb_Arena_Malloc, which memcpys the value into it, and the map ends up
with a silently corrupted key holding unrelated heap bytes. The key is
tagged UTF-8 while containing invalid UTF-8, so it then propagates into
encode and to_json.
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 the precondition
Convert_StringData's comment describes. Reword that comment to say so,
since it read as if the aliasing were unconditionally safe.
Reproduces under ordinary GC without GC.stress: one corrupted key across
150k iterations, versus 100/100 with stress. Trigger requires a key that
is a Symbol or a non-UTF-8 String (ASCII-8BIT is the common case for
anything read from a socket, file, Marshal or pack) together with a
value whose conversion allocates. Plain UTF-8 keys are unaffected.
Adds regression tests covering string keys, Symbol keys, and the
map-field kwarg path. They fail on the unpatched extension and pass with
this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #29023.
Map#[]=andMessage.new(map_field: {...})build the map key as aupb_StringViewaliasing a Ruby String, then convert the value before
upb_Map_Setcopies the key. Thevalue conversion allocates, so it can trigger GC inside that window.
The aliased String is frequently a temporary:
Convert_RubyToUpbreplaces the caller'sobject when the key is a Symbol (via
to_s) or a String not already tagged UTF-8 (viaConvert_CheckStringUtf8), and nothing references the result once it returns. When GCcollects it, the freed block goes straight back to the next
upb_Arena_Malloc, whichmemcpys 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 theNULLfast path — theyconsume 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 thoughthe 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_StringDataat all.Trigger
Needs both:
ASCII-8BITis thecommon case for anything read from a socket, a file,
Marshal, orString#pack; andPlain UTF-8 keys are unaffected, which is presumably why this has gone unnoticed.
Verification
Reproduces under ordinary GC, no
GC.stressrequired — one corrupted key across 150kiterations (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.rbcovering string keys, Symbol keys, and themap-field kwarg path. Verified red/green against the same tree:
mainFull 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 inan OT0 repository.