Root the aggregate argument array so GC cannot free live values (GHSA-mwm8-39rw-8826) - #733
Merged
Conversation
…-mwm8-39rw-8826) rb_sqlite3_aggregator_step converts an aggregate's SQLite arguments into a VALUE array allocated with xcalloc. That memory is not a GC root. sqlite3val2rb allocates, so a GC triggered while converting a later argument can collect a Ruby object already stored in an earlier slot and reuse it -- the step block then receives a wrong or freed object, and above Ruby 3.4's 640-byte embedded boundary (where every conversion mallocs) the process can segfault under ordinary GC. Needs arity >= 2: the argc == 1 branch stores into a stack local, which conservative stack scanning already pins. Fix: register each slot with rb_gc_register_address before filling any, so a GC during a later conversion scans every VALUE already stored, and unregister before xfree. This is the idiom nokogiri uses at xml_xpath_context.c. Distinct from #723, which pins the callback instances stored as SQLite user-data and does not touch this argument array. Both are needed. Verified on the released precompiled artifact's source, ruby 3.4.10 arm64-darwin, system SQLite, two separately built extensions: GC.stress, arity 2 before CORRUPT 3/3 after CLEAN 3/3 ordinary GC, ~4KB rows before SIGSEGV after CLEAN 8/8 (incl. the size that crashed before) arity 1 (control) CLEAN both patched under GC.stress CLEAN test_multi_argument_step_arguments_survive_gc reproduces it: the step block checks each argument against the column it came from, under GC.stress. It fails on an unpatched build (a slot holds an unrelated object) and passes with this change.
Rooting the aggregate argument array with rb_gc_register_address made each step call pay a linear scan of the VM's global-address list per slot on unregister, and a raise from sqlite3val2rb mid-fill would have leaked the array and left its slots registered as permanent roots. The argument array will be allocated with ALLOCV_N, whose memory is conservatively marked (machine stack up to 1KB, a GC-managed tmpbuf above that), so stored VALUEs survive a GC during conversion at no per-slot cost, and the buffer is reclaimed by GC if the fill raises. This follows sparklemotion/nokogiri@ab050ed0, which replaced the same register-address idiom in xml_xpath_context.c.
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
2 participants
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.
See GHSA-mwm8-39rw-8826.