fix(gc): root fs.readdir's options object across the withFileTypes key allocation (#7274) - #7693
Conversation
📝 WalkthroughWalkthroughThe filesystem options decoder now roots option objects across key allocation, refreshes relocated pointers, and centralizes pointer validation. Runtime-root tests cover moving collections for present and absent ChangesFilesystem options GC safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/fs/dirent.rs`:
- Around line 140-153: Update options_object_ptr to validate raw_ptr with the
canonical crate::value::addr_class::is_plausible_heap_addr predicate before
converting it to an ObjectHeader pointer. Replace the existing
is_handle_band-only rejection while preserving the current None return for
invalid addresses.
In `@crates/perry-runtime/src/gc/tests/runtime_roots/fs_options_object.rs`:
- Around line 152-160: Update the negative-test setup after js_object_alloc in
the relevant test to assert pointer_in_nursery(obj as usize), matching the
positive test’s nursery-membership check before collection. Keep the existing
options-field initialization and relocation sentinel unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81cbdbd8-e808-416a-8b74-bda5f9cb4cda
📒 Files selected for processing (4)
changelog.d/7693-readdir-options-rooting.mdcrates/perry-runtime/src/fs/dirent.rscrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/gc/tests/runtime_roots/fs_options_object.rs
| unsafe fn options_object_ptr(options_value: f64) -> Option<*const crate::object::ObjectHeader> { | ||
| let bits = options_value.to_bits(); | ||
| let value = crate::value::JSValue::from_bits(bits); | ||
| let raw_ptr = if value.is_pointer() { | ||
| value.as_pointer::<crate::object::ObjectHeader>() as usize | ||
| } else if bits >> 48 == 0x0000 { | ||
| (bits & 0x0000_FFFF_FFFF_FFFF) as usize | ||
| } else { | ||
| return false; | ||
| return None; | ||
| }; | ||
| // #7259: a POINTER_TAG payload can be a registry handle id rather than a | ||
| // heap address, and `< 0x1000` sits an order of magnitude below | ||
| // `HANDLE_BAND_MAX` — fetch/zlib/proxy ids passed it and were dereferenced | ||
| // as an ObjectHeader (the Linux-only fault class of #1843/#4004/#6271). | ||
| // `is_handle_band` also subsumes the null check that used to follow. | ||
| if crate::value::addr_class::is_handle_band(raw_ptr) { | ||
| return false; | ||
| return None; | ||
| } | ||
| let obj_ptr = raw_ptr as *const crate::object::ObjectHeader; | ||
| let key = crate::string::js_string_from_bytes(b"withFileTypes".as_ptr(), 13); | ||
| Some(raw_ptr as *const crate::object::ObjectHeader) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the canonical heap-address predicate.
Line 150 rejects only the handle band. Replace this check with crate::value::addr_class::is_plausible_heap_addr(raw_ptr) before converting raw_ptr to *const ObjectHeader. This shared decoder can otherwise pass non-heap raw addresses to field access and cause an invalid dereference.
Proposed fix
- if crate::value::addr_class::is_handle_band(raw_ptr) {
+ if !crate::value::addr_class::is_plausible_heap_addr(raw_ptr) {
return None;
}Based on learnings: “use the canonical predicate crate::value::addr_class::is_plausible_heap_addr for the handle-band/heap-floor check.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe fn options_object_ptr(options_value: f64) -> Option<*const crate::object::ObjectHeader> { | |
| let bits = options_value.to_bits(); | |
| let value = crate::value::JSValue::from_bits(bits); | |
| let raw_ptr = if value.is_pointer() { | |
| value.as_pointer::<crate::object::ObjectHeader>() as usize | |
| } else if bits >> 48 == 0x0000 { | |
| (bits & 0x0000_FFFF_FFFF_FFFF) as usize | |
| } else { | |
| return false; | |
| return None; | |
| }; | |
| // #7259: a POINTER_TAG payload can be a registry handle id rather than a | |
| // heap address, and `< 0x1000` sits an order of magnitude below | |
| // `HANDLE_BAND_MAX` — fetch/zlib/proxy ids passed it and were dereferenced | |
| // as an ObjectHeader (the Linux-only fault class of #1843/#4004/#6271). | |
| // `is_handle_band` also subsumes the null check that used to follow. | |
| if crate::value::addr_class::is_handle_band(raw_ptr) { | |
| return false; | |
| return None; | |
| } | |
| let obj_ptr = raw_ptr as *const crate::object::ObjectHeader; | |
| let key = crate::string::js_string_from_bytes(b"withFileTypes".as_ptr(), 13); | |
| Some(raw_ptr as *const crate::object::ObjectHeader) | |
| unsafe fn options_object_ptr(options_value: f64) -> Option<*const crate::object::ObjectHeader> { | |
| let bits = options_value.to_bits(); | |
| let value = crate::value::JSValue::from_bits(bits); | |
| let raw_ptr = if value.is_pointer() { | |
| value.as_pointer::<crate::object::ObjectHeader>() as usize | |
| } else if bits >> 48 == 0x0000 { | |
| (bits & 0x0000_FFFF_FFFF_FFFF) as usize | |
| } else { | |
| return None; | |
| }; | |
| if !crate::value::addr_class::is_plausible_heap_addr(raw_ptr) { | |
| return None; | |
| } | |
| Some(raw_ptr as *const crate::object::ObjectHeader) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/fs/dirent.rs` around lines 140 - 153, Update
options_object_ptr to validate raw_ptr with the canonical
crate::value::addr_class::is_plausible_heap_addr predicate before converting it
to an ObjectHeader pointer. Replace the existing is_handle_band-only rejection
while preserving the current None return for invalid addresses.
Source: Learnings
| let obj = crate::object::js_object_alloc(0, 1); | ||
| let key = crate::string::js_string_from_bytes(b"encoding".as_ptr(), 8); | ||
| let encoding = crate::string::js_string_from_bytes(b"utf8".as_ptr(), 4); | ||
| crate::object::js_object_set_field_by_name( | ||
| obj, | ||
| key, | ||
| f64::from_bits(string_bits(encoding as usize)), | ||
| ); | ||
| let options_value = f64::from_bits(ptr_bits(obj as usize)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the negative-test options object is movable.
The sentinel proves that the minor collection moved, but it does not prove that obj was in the nursery. Add pointer_in_nursery(obj as usize) after allocation, as the positive test does. Without this assertion, the test can pass when the options object does not relocate.
Proposed fix
let obj = crate::object::js_object_alloc(0, 1);
+ assert!(
+ crate::arena::pointer_in_nursery(obj as usize),
+ "the options object must be movable or this test does not exercise relocation"
+ );
let key = crate::string::js_string_from_bytes(b"encoding".as_ptr(), 8);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let obj = crate::object::js_object_alloc(0, 1); | |
| let key = crate::string::js_string_from_bytes(b"encoding".as_ptr(), 8); | |
| let encoding = crate::string::js_string_from_bytes(b"utf8".as_ptr(), 4); | |
| crate::object::js_object_set_field_by_name( | |
| obj, | |
| key, | |
| f64::from_bits(string_bits(encoding as usize)), | |
| ); | |
| let options_value = f64::from_bits(ptr_bits(obj as usize)); | |
| let obj = crate::object::js_object_alloc(0, 1); | |
| assert!( | |
| crate::arena::pointer_in_nursery(obj as usize), | |
| "the options object must be movable or this test does not exercise relocation" | |
| ); | |
| let key = crate::string::js_string_from_bytes(b"encoding".as_ptr(), 8); | |
| let encoding = crate::string::js_string_from_bytes(b"utf8".as_ptr(), 4); | |
| crate::object::js_object_set_field_by_name( | |
| obj, | |
| key, | |
| f64::from_bits(string_bits(encoding as usize)), | |
| ); | |
| let options_value = f64::from_bits(ptr_bits(obj as usize)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/gc/tests/runtime_roots/fs_options_object.rs` around
lines 152 - 160, Update the negative-test setup after js_object_alloc in the
relevant test to assert pointer_in_nursery(obj as usize), matching the positive
test’s nursery-membership check before collection. Keep the existing
options-field initialization and relocation sentinel unchanged.
0bd8985 to
57da27f
Compare
…y allocation (#7274) `fs/dirent.rs::options_with_file_types` decoded a raw `*const ObjectHeader` out of the NaN-boxed `options` argument, THEN called `js_string_from_bytes(b"withFileTypes")` — a collection point — and THEN dereferenced the address it had computed before the collection. `options_value` is a plain Rust `f64` local, so nothing kept the object alive and nothing rewrote the pointer. The allocation is now hoisted above the decode and the options value is rooted in a `RuntimeHandleScope`, so the address only ever comes back out of a slot the collector rewrote — the shape `options_field_value` 40 lines below already used. The decode itself (POINTER_TAG / raw-address forms plus the #7259 handle-band floor) is factored into one `options_object_ptr` helper, because this file performed it in three places and the drift between two of them is what this bug was. Witness: two knob-free unit tests in `gc/tests/runtime_roots/fs_options_object.rs` drive a real evacuating minor from inside the function's own key allocation. The options object is held by nothing else, and a separately rooted sentinel must come back at a different address — so a cycle that moved nothing cannot certify the file. Sabotage-verified: restoring the pre-fix decode-then-allocate order fails `readdir_options_object_survives_the_with_file_types_key_allocation`.
Audit — merging as v0.5.1398Sabotage-verified independently: restoring the pre-fix order (decode → allocate → deref) reddens The framing I want on the record is yours: "the bug is the drift between the two." Hoisting the allocation above the decode and binding it with
Gates: 24/24 lint, fmt clean, |
57da27f to
dd78ae1
Compare
* fix(gc): restore the raw-handle debt baseline after #7693 #7693's test asserts a pre/post address comparison, the shape across_* exists to make unnameable, so it is listed rather than converted. Four pairs converted elsewhere to hold the baseline at 998. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1399 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Closes #7274.
The defect
crates/perry-runtime/src/fs/dirent.rs::options_with_file_typesdecoded a raw*const ObjectHeaderout of the NaN-boxedoptionsargument, then calledjs_string_from_bytes(b"withFileTypes")— a collection point — and thendereferenced the address it had computed before the collection.
options_valueis a plain Rust
f64local, so nothing kept the object alive and nothingrewrote the pointer.
{ withFileTypes: true }is a fresh object literal at the call site, i.e. anursery object — precisely the generation an evacuating minor relocates.
options_field_value, 40 lines below in the same file, has the same signatureand already did it correctly. The bug is the drift between the two.
The fix
RuntimeHandle::across_nanboxso there is no pre-collection address in scopeto reach for by accident, and
options_valueis rooted in aRuntimeHandleScope.is_handle_bandfloor — is factored into oneoptions_object_ptrhelper.It appeared three times in this file; two copies drifting is what this bug
was, so the duplication is removed rather than fixed twice.
No signature change, no caller churn.
Which configuration this actually bites in
Stated precisely, because the honest answer is narrower than "shipped default
segfaults" and the test file says so out loud:
gc_check_trigger's alloc-point arm engagesManualGcScanGuard::force_full_scan(NurseryChurnSlackValve)— unconditionalsince #7682. When that guard engages, the conservative native-stack scan
both retains the raw local and makes the copying minor ineligible
(
CopiedMinorFallbackReason::ConservativeStack), so the pre-fix code survives:the address neither dies nor moves.
It does not always engage.
force_full_scanis a no-op whenCONSERVATIVE_STACK_SCAN_OVERRIDEis already set, andconservative_stack_scan_modelets an explicitPERRY_CONSERVATIVE_STACK_SCANenv value beat any pin. So
PERRY_CONSERVATIVE_STACK_SCAN=off— the arm everyissue in this family reproduces on — removes the valve and the alloc-point minor
evacuates.
So: the masking mechanism is the bounded valve #7148 documents as a bounded
valve, not a guarantee. This PR closes the hazard rather than relying on it.
Witness
Two knob-free unit tests in
crates/perry-runtime/src/gc/tests/runtime_roots/fs_options_object.rs. Theydrive a real evacuating minor from inside the function's own key allocation
and run in exactly the configuration described above (
CopyingNurseryTestGuardpins the scan mode to
Auto, which resolves toSkipDisabled— the samedecision
PERRY_CONSERVATIVE_STACK_SCAN=offproduces).The subject is asserted live, per CLAUDE.md's "a gate must assert its subject
was live":
shadow slot, no registered root;
at a different address, so a cycle that moved nothing cannot certify the file.
That assertion earned its place immediately: the first draft pinned
force_legacy_gc_pacing()(scavenge off, polls off), which routes the trigger tothe budgeted stepper — non-moving by construction. The sentinel did not move and
the test failed rather than passing vacuously. Pacing is now left at the shipped
default, deliberately, and the file records why.
Sabotage
Reverting
options_with_file_typesto the pre-fix decode-then-allocate order:The negative test stays green under sabotage — a stale read yields not-truthy
just as readily as a correct one. It is labelled
NOT A DISCRIMINATORin thesource so its passing is never read as evidence about the rooting; it guards the
None/false arms of the rewritten decode, which the positive test never reaches.Gates run locally
cargo fmt --all -- --check,cargo test -p perry-runtime --lib,check_file_size.sh,check_test_registration.py,global_sink_isolation.py,addr_class_inventory.py— all green.Summary by CodeRabbit
Bug Fixes
fs.readdirhandling whenwithFileTypesoptions are processed during garbage collection.withFileTypes.Tests
fs.readdirprocessing.