fix(gc): a Symbol's description was stored into the header already stale - #7376
Conversation
alloc_symbol calls gc_malloc -- a collection point -- and then writes the description pointer its caller computed BEFORE that call. An evacuating minor moves the description string, so a live SymbolHeader holds a retired from-space address and js_symbol_to_string faults reading it through str_from_header. Same shape and same fix as RegExpHeader::flags_ptr (#7374): root across the allocation, re-read after. 3/3 cluster tests clean, byte-identical to Node. LEFT OPEN DELIBERATELY: the header is allocated GC_TYPE_STRING, whose payload the collector treats as opaque, so a fresh symbol's description is never marked or rewritten after construction. The comment at the site already says this -- 'kept alive through the SYMBOL_REGISTRY (for registered symbols) or not at all (for fresh symbols ... which is fine for test workloads)'. This makes the STORED value correct; keeping it alive for the symbol's lifetime is a separate fix, noted in the changelog and tracked in #7341.
js_object_literal_infer_computed_function_name was triaged as a separate cluster because its backtrace names a different frame. It reaches the same stale (*sym_ptr).description through infer_symbol_function_name, so the #7376 fix closes it too -- verified 3/3 clean, byte-identical to Node. Two distinct faulting frames, one root cause. Worth recording: grouping catches by frame #0 is the right first cut, but it over-counts clusters whenever one bad field has several readers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSymbol description GC safety
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…r it was written (#7378) #7341's quarantine produced 31 real stale-pointer bugs; four were fixed (#7373-#7376). None existed when the RFC's 'would it have caught the real bugs' table was written, so they are the strongest available calibration. It would have caught ONE of the four. #7375 (await polling a moved promise) is squarely in scope and would have been caught completely -- notably it had survived a comment explicitly reasoning about the surrounding hazard, which is the RFC's own central argument. The other three lived in perry-runtime, which this RFC does not govern. Recording that honestly rather than only the win, because the ratio is the useful part: it says where the remaining risk lives. Three of four were layer 3, where RuntimeHandleScope exists (675 uses) but is optional. The sharper finding is that all four were the SAME defect shape -- ordering a root relative to a collection point, never a missing root. That is exactly what a Raw that dies at the next &mut emit enforces, so the design generalises; the open question is whether layer 3 needs the same discipline rather than whether codegen does. Also notes the sample is favourable: the eight catches left open in #7341 are caller-side, and Raw<'e> does not cross a function boundary either. No code change -- the RFC says step 1 wants a quiet tree, and expr/ is under edit by #7375. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Closes 6 of 31 catches in #7341 — two apparent clusters, one root cause.
The bug
alloc_symbolcallsgc_malloc— a collection point — and then writes the description pointer its caller computed before that call:An evacuating minor moves the description string, so a live
SymbolHeaderholds a retired from-space address. Same shape and same fix asRegExpHeader::flags_ptr(#7374): root across the allocation, re-read after.Two readers, two apparent clusters
js_symbol_to_stringreaches the stale field directly.infer_symbol_function_namereaches it throughjs_object_literal_infer_computed_function_name, which had been triaged as a separate cluster because its backtrace names a different frame — and which this fix closes too (3/3 verified).Worth stating as method: grouping catches by frame #0 is the right first cut, but it over-counts clusters whenever one bad field has several readers.
A related gap left open deliberately
The header is allocated
GC_TYPE_STRING, whose payload the collector treats as opaque — so a fresh (non-registered) symbol's description is never marked or rewritten after construction. The existing comment says so:This change makes the stored value correct. It does not make the description live. That is a latent use-after-free and it is called out in the changelog and commit rather than left for the green tests to imply otherwise. Tracked in #7341.
Verification
6/6 affected tests clean, byte-identical to Node.
The 2
symbol-filtered unit-test failures seen on this branch are pre-existing flake (#7365), confirmed by running the same filter 4× on each side: clean main gives 1/3/2/2 failures, this branch gives 1/0/0/1.Summary by CodeRabbit