fix(runtime): #5901 — strict write to a frozen symbol-keyed property throws TypeError - #5958
Merged
Merged
Conversation
…throws
A strict-mode `obj[sym] = v` where `sym` is a Symbol routes through
`js_put_value_set` → `ordinary_set_with_receiver` → `own_set_descriptor`.
For a symbol key that helper returned `Data { writable: true }`
unconditionally whenever the property existed, ignoring the receiver's frozen
state and any per-symbol `writable:false` attribute. So `Object.freeze(obj);
obj[sym] = 2` silently no-op'd instead of throwing the required TypeError
(test262 Object/freeze/frozen-object-contains-symbol-properties-strict) — the
string-keyed path already reported these correctly.
Fix: `own_set_descriptor` now reports a symbol-keyed data property's real
writability via a new `symbol::symbol_property_is_non_writable` query, which
mirrors the frozen / per-symbol-attr rejection already in
`set_symbol_property` (frozen receiver ⇒ non-writable; else consult the
per-symbol attrs table). `ordinary_set_with_receiver` then returns false and
`js_put_value_set` throws under strict mode.
test262 built-ins/Object/freeze: slice now 0 fail. Verified against Node:
normal symbol overwrite, a `defineProperty(obj, sym, {writable:false})` strict
write (throws), a sealed-but-not-frozen object's existing symbol (still
writable), and `Symbol.iterator` all behave correctly.
Refs #5901.
📝 WalkthroughWalkthroughAdds a new ChangesSymbol Writability Check
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/symbol/properties.rs (1)
375-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared "is receiver frozen" check.
The GC-flag frozen check here (Lines 382-391) duplicates the equivalent block in
set_symbol_property(Lines 311-320 in the unchanged context). Both must independently deriveobj_key/sym_key, cast toGcHeader, and testOBJ_FLAG_FROZEN. Since this PR exists precisely because two related code paths drifted out of sync (writability logic wasn't mirrored here originally), extracting a sharedfn is_heap_receiver_frozen(obj_f64: f64, obj_key: usize) -> boolhelper would reduce the chance of the same divergence recurring.♻️ Suggested extraction
+fn heap_receiver_is_frozen(obj_f64: f64, obj_key: usize) -> bool { + if (obj_f64.to_bits() >> 48) == 0x7FFD + && obj_key >= 0x10000 + && crate::object::is_valid_obj_ptr(obj_key as *const u8) + { + let gc = (obj_key - crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; + return unsafe { (*gc)._reserved } & crate::gc::OBJ_FLAG_FROZEN != 0; + } + false +} + pub(crate) fn symbol_property_is_non_writable(obj_f64: f64, sym_f64: f64) -> bool { let obj_key = unsafe { obj_key_from_f64(obj_f64) }; let sym_key = unsafe { sym_key_from_f64(sym_f64) }; if obj_key == 0 || sym_key == 0 { return false; } - // Only heap receivers carry the GC integrity flag word. - if (obj_f64.to_bits() >> 48) == 0x7FFD - && obj_key >= 0x10000 - && crate::object::is_valid_obj_ptr(obj_key as *const u8) - { - let gc = (obj_key - crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; - let flags = unsafe { (*gc)._reserved }; - if flags & crate::gc::OBJ_FLAG_FROZEN != 0 { - return true; - } - } + if heap_receiver_is_frozen(obj_f64, obj_key) { + return true; + } get_symbol_property_attrs(obj_key, sym_key).is_some_and(|attrs| !attrs.writable()) }🤖 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/symbol/properties.rs` around lines 375 - 393, The frozen-receiver GC flag check in symbol_property_is_non_writable is duplicated from set_symbol_property and can drift again; extract the shared heap-receiver frozen test into a helper such as is_heap_receiver_frozen and reuse it in both code paths. Keep the existing obj_key/sym_key derivation in symbol_property_is_non_writable, but move the GcHeader cast, OBJ_FLAG_FROZEN read, and heap-validity guard into the shared helper so both writability and mutability logic stay aligned.
🤖 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/proxy.rs`:
- Around line 901-913: The symbol-own-property branch in proxy.rs is using
js_object_get_symbol_property() and TAG_UNDEFINED to detect presence, which
incorrectly treats an existing symbol property whose value is undefined as
missing. Update this logic in the same area as OwnSetDescriptor::Data to use the
symbol existence check already used by the setter path, such as
object_symbol_data_property_exists or own_symbol_property, and then derive
writability from that result instead of inspecting the returned value bits.
---
Nitpick comments:
In `@crates/perry-runtime/src/symbol/properties.rs`:
- Around line 375-393: The frozen-receiver GC flag check in
symbol_property_is_non_writable is duplicated from set_symbol_property and can
drift again; extract the shared heap-receiver frozen test into a helper such as
is_heap_receiver_frozen and reuse it in both code paths. Keep the existing
obj_key/sym_key derivation in symbol_property_is_non_writable, but move the
GcHeader cast, OBJ_FLAG_FROZEN read, and heap-validity guard into the shared
helper so both writability and mutability logic stay aligned.
🪄 Autofix (Beta)
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: 0d09c6a6-16f0-445a-89af-caf5472eabee
📒 Files selected for processing (3)
crates/perry-runtime/src/proxy.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/symbol/properties.rs
Comment on lines
901
to
913
| if unsafe { crate::symbol::js_is_symbol(key) } != 0 { | ||
| let value = unsafe { crate::symbol::js_object_get_symbol_property(target, key) }; | ||
| return (value.to_bits() != TAG_UNDEFINED) | ||
| .then_some(OwnSetDescriptor::Data { writable: true }); | ||
| if value.to_bits() == TAG_UNDEFINED { | ||
| return None; | ||
| } | ||
| // An existing symbol-keyed own data property is non-writable when the | ||
| // receiver is frozen or its per-symbol attrs say so — so a strict | ||
| // `obj[sym] = v` is rejected (throws) rather than silently no-op'd | ||
| // (test262 Object/freeze/frozen-object-contains-symbol-properties-strict). | ||
| // Mirrors the string-keyed / `set_symbol_property` guards. | ||
| let writable = !crate::symbol::symbol_property_is_non_writable(target, key); | ||
| return Some(OwnSetDescriptor::Data { writable }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "fn js_object_get_symbol_property" -A 20 crates/perry-runtime/src/symbol
rg -n "fn object_symbol_data_property_exists" -A 10 crates/perry-runtime/src/symbolRepository: PerryTS/perry
Length of output: 3248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '240,340p' crates/perry-runtime/src/symbol/get.rs
printf '\n==== proxy.rs ====\n'
sed -n '880,940p' crates/perry-runtime/src/proxy.rs
printf '\n==== properties.rs ====\n'
sed -n '340,390p' crates/perry-runtime/src/symbol/properties.rsRepository: PerryTS/perry
Length of output: 10316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "own_set_descriptor\(" crates/perry-runtime/src/proxy.rs crates/perry-runtime/src -g '!target'
printf '\n==== symbol property writes ====\n'
rg -n "store_object_symbol_property_root|object_symbol_data_property_exists|symbol_property_is_non_writable|js_object_get_symbol_property" crates/perry-runtime/src/symbol crates/perry-runtime/src -g '!target'
printf '\n==== assignment path around own_set_descriptor usage ====\n'
sed -n '940,1080p' crates/perry-runtime/src/proxy.rsRepository: PerryTS/perry
Length of output: 17423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '==== js_object_get_symbol_property internals ====\\n'
sed -n '1,220p' crates/perry-runtime/src/symbol/get.rs
printf '\\n==== own_set_descriptor call sites ====\\n'
sed -n '1110,1165p' crates/perry-runtime/src/proxy.rs
printf '\\n==== later own_set_descriptor call site ====\\n'
sed -n '1290,1338p' crates/perry-runtime/src/proxy.rs
printf '\\n==== symbol data property tests / docs ====\\n'
rg -n "undefined|TAG_UNDEFINED|object_symbol_data_property_exists|defineProperty\\(obj, sym" crates/perry-runtime/src/symbol crates/perry-runtime/src/object -g '!target'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "own_symbol_property|pub use .*object_symbol_data_property_exists|pub use .*own_symbol_property|object_symbol_data_property_exists" crates/perry-runtime/src/symbol.rs crates/perry-runtime/src/symbol crates/perry-runtime/src/proxy.rsRepository: PerryTS/perry
Length of output: 1895
Use an existence check for symbol own-properties here
js_object_get_symbol_property() can return TAG_UNDEFINED for a property that exists with value undefined, so this branch still conflates “missing” with “present but undefined”. That lets an extensible receiver fall through to the create/new-property path instead of treating the existing symbol data property as read-only. Use the same presence check as the symbol setter path (object_symbol_data_property_exists / own_symbol_property) instead of testing the returned value bits.
🤖 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/proxy.rs` around lines 901 - 913, The
symbol-own-property branch in proxy.rs is using js_object_get_symbol_property()
and TAG_UNDEFINED to detect presence, which incorrectly treats an existing
symbol property whose value is undefined as missing. Update this logic in the
same area as OwnSetDescriptor::Data to use the symbol existence check already
used by the setter path, such as object_symbol_data_property_exists or
own_symbol_property, and then derive writability from that result instead of
inspecting the returned value bits.
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
1 participant
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.
Summary
A strict-mode
obj[sym] = v(Symbol key) onto a frozen object silently no-op'd instead of throwing aTypeError. FixesObject/freeze/frozen-object-contains-symbol-properties-strictfrom #5901.Root cause
Strict-mode
obj[sym] = vroutes throughjs_put_value_set→ordinary_set_with_receiver→own_set_descriptor. For a symbol key,own_set_descriptorreturnedData { writable: true }unconditionally whenever the property existed — ignoring the receiver's frozen state and any per-symbolwritable:falseattribute. So the ordinary[[Set]]reported success, andjs_put_value_setnever threw. (The string-keyed path already reported these correctly.)Fix
own_set_descriptornow reports a symbol-keyed data property's real writability via a newsymbol::symbol_property_is_non_writablequery, which mirrors the frozen / per-symbol-attr rejection already present inset_symbol_property:defineProperty(obj, sym, {writable:false})).ordinary_set_with_receiverthen returnsfalse, andjs_put_value_setthrows under strict mode. A sealed-but-not-frozen object's existing symbol property stays writable (sealed permits value changes), so only genuine non-writable slots are rejected.Before / after (test262
built-ins/Object)freezesub-slice: 0 fail (frozen-object-contains-symbol-properties-strictwas the sole failure there).Verified against Node that: a normal symbol overwrite, a
defineProperty(obj, sym, {writable:false})strict write (throws), a sealed-but-not-frozen object's existing symbol (still writable), andSymbol.iteratoraccess all behave identically. No behavioral change for non-symbol keys or non-frozen receivers — the change is scoped to the symbol branch ofown_set_descriptor.Validation
cargo fmt --all -- --checkclean;scripts/check_file_size.shclean (proxy.rs 1972, properties.rs 626 lines). Built and tested on an internal Linux box.Refs #5901.
Summary by CodeRabbit
ProxyandReflect.setbehavior more accurate.