Fix a crash when reading a method off a Buffer without calling it

[jdalton]

Fixing a crash when you read a method off a Buffer

Reading a method from a Buffer without calling it — typeof buf.readUInt8, const f = buf.readUInt8, or buf[someKey] — produced a function that pointed at memory which had already been freed or moved. Calling it later read whatever happened to be sitting there.

This fixes the segmentation fault on test_gap_buffer_own_props and test_gap_buffer_own_prop_shadow_intrinsic_6405 in the conformance test suite. Neither test is listed in gap_snapshot.json, meaning both are expected to pass.

What was wrong

When you read a method off a Buffer but do not call it, Perry has to hand you back something callable. It builds a small object that remembers two things: which Buffer you read it from, and the name of the method. When you eventually call it, that name is looked up and the real method runs.

The catch is that it remembers the name as a pointer — an address in memory — rather than copying the text. So whatever it points at has to stay valid and unchanged for as long as that callable object might be used. The function's own documentation says exactly this:

Method-name pointer is expected to be stable for the closure's lifetime; codegen emits it from the per-module .str.N.bytes rodata global.

The compiler holds up its end: the names it passes live in a read-only section of the executable and never move. Two places in the runtime did not:

where	what it pointed at	why that breaks
get_field_by_name_tail	inside the key string on the garbage-collected heap	that string becomes unreachable the moment the read returns, so the collector is free to reclaim it or relocate it while the callable is still around
polymorphic_index, the buf[key] path	inside a temporary Rust String local	that memory is freed when the function returns, so the pointer is dangling immediately — no garbage collector involved at all

In both cases you got back something that looked like a working function and was, in fact, pointing at memory nobody owned any more.

The fix

Both places now pass a pointer to a string literal baked into the executable, which is never freed and never moves — the same guarantee the compiler already provides.

To make that possible without maintaining a second copy of the list of Buffer method names, the existing list in buffer_dispatch now generates two things from one source: the existing yes/no check, and a new lookup that returns the matching literal. The yes/no check compiles to exactly what it did before, and the new lookup is only reached on the path that was already about to allocate, so nothing on a hot path got slower.

I checked the other callers of the same function while I was in there. The ones in symbol/get.rs pass byte-string literals, which already live in read-only memory, and the three in descriptors.rs and the class registry already make a permanent copy. These two were the only ones getting it wrong.

Why this looked like a flaky test rather than a bug

Whether reading freed memory actually goes wrong depends on whether anything has reused that memory yet. That is a property of the memory allocator on the machine you happen to be running on — not a property of the program.

So this passed consistently on macOS while segfaulting on Linux in CI. Worse, it looked like unrelated changes were causing it: an in-flight pull request that made the garbage collector run more often flipped test_gap_buffer_own_props from passing to crashing without touching a single line of Buffer code. More collections meant more churn, which meant the freed bytes were more likely to have been reused by the time anything read them.

That is exactly the kind of bug that gets triaged as "flaky, re-run it".

The tests, and why they check the shape rather than the symptom

The obvious test — call the method after forcing a collection and see whether it still works — is a bad test here, because it asks the machine's allocator a question rather than asking the program one. It would pass on a lucky host with the bug fully present, which is the failure mode that let this ship in the first place.

So the three tests in gc/tests/buffer_bound_method_name.rs check the underlying rule instead:

1. The callable's remembered name must not point inside the key string.
2. The buf[key] path's remembered name must not point at a temporary.
3. The new lookup must not return a borrow of what you passed it, and two callers looking up the same name must get back the identical pointer, no matter where their own copy of the text lives.

These live under gc/tests/ so they run in the required per-pull-request cargo-test job, rather than the nightly-only tier where a regression could sit red for days.

They were checked by deliberately breaking the fix. With the two corrected call sites reverted and the tests left alone, the first two fail — and the second one fails because the remembered name had already turned into garbage bytes. So the use-after-free reproduces deterministically, in-process, on the same machine where the end-to-end tests were happily passing.

How this was verified

• cargo test -p perry-runtime --lib — 1979 passed, 0 failed, 4 ignored.
• All 12 Buffer and DataView conformance tests produce byte-identical output to Node 26.5.1, including the two that were crashing.
• scripts/check_file_size.sh passes.

No version bump — the maintainer bumps that at merge time, per the external-contributor flow.

Summary by CodeRabbit

• Bug Fixes

  • Fixed Buffer bound methods to remain reliable when accessed directly or through computed property names.
  • Prevented failures caused by temporary or changing property-name storage.
  • Preserved expected Buffer method behavior while continuing to reject unknown method names.

• Tests

  • Added regression coverage for method-name stability and Buffer method lookup behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 96b1bda9-50a5-4b75-8a3e-d3c83046185e

📥 Commits

Reviewing files that changed from the base of the PR and between c1c4524 and 62d069e.

📒 Files selected for processing (1)

• crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs

---

📝 Walkthrough

Walkthrough

Buffer method lookup now binds stable static method-name literals in property and computed-key paths. A shared macro generates method recognition and static-name lookup. GC tests verify pointer ownership and lifetime.

Changes

Buffer method-name lifetime

Layer / File(s)	Summary
Static method-name registry crates/perry-runtime/src/object/buffer_dispatch.rs	A shared macro generates Buffer/DataView method recognition and lookup of stable static method-name literals.
Bound-method integration crates/perry-runtime/src/object/field_get_set/buffer_own_prop.rs, crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs, crates/perry-runtime/src/object/polymorphic_index.rs	Buffer property and polymorphic lookup paths pass static method-name storage instead of pointers into temporary key strings.
Lifetime regression coverage crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs, crates/perry-runtime/src/gc/tests/mod.rs, changelog.d/7747-buffer-bound-method-name-lifetime.md	Tests cover direct lookup, computed-key lookup, static-literal ownership, and unknown methods. The test module is registered, and the changelog records the fix and test results.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• PerryTS/perry#6842: Addresses stable method or property dispatch identifiers and lifetime safety in different runtime paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the Buffer bound-method crash fix.
Description check	✅ Passed	The description thoroughly explains the bug, fix, affected paths, tests, and versioning status, despite omitting some template headings.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/gc/tests/buffer_bound_method_name.rs`:
- Around line 87-95: Update the computed-key test assertions around the captured
method name to compare name_ptr against
buffer_method_name_static("readUInt8").unwrap().as_ptr() rather than
key_interior. Keep the existing byte-content assertion and ensure the test
verifies capture of the static literal pointer.

🪄 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: 8fcf067b-b755-4b10-98dc-36d8c531b75c

📥 Commits

Reviewing files that changed from the base of the PR and between b2aef50 and c1c4524.

📒 Files selected for processing (7)

• changelog.d/7747-buffer-bound-method-name-lifetime.md
• crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs
• crates/perry-runtime/src/gc/tests/mod.rs
• crates/perry-runtime/src/object/buffer_dispatch.rs
• crates/perry-runtime/src/object/field_get_set/buffer_own_prop.rs
• crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs
• crates/perry-runtime/src/object/polymorphic_index.rs

[coderabbitai]

✅ Action performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

[proggeramlug]

Merging as v0.5.1437 — a real use-after-free, and the diagnosis is exactly right

polymorphic_index passed name.as_bytes().as_ptr() where name is a local String. That pointer dangles the moment the function returns — no collector involved. get_field_by_name_tail's was subtler but the same class: a borrow into a GC-heap key string that becomes unreachable as soon as the read returns, so the collector may reclaim or relocate it while the callable is still live.

Both violate a contract the function's own doc already stated: "Method-name pointer is expected to be stable for the closure's lifetime; codegen emits it from the per-module .str.N.bytes rodata global." The compiler held up its end; two runtime sites did not.

The single-source macro is the right shape. $name => Some($name) returns the pattern literal — a &'static str baked into the binary — rather than a borrow of the input. That is the whole trick, and it means the yes/no check and the lookup cannot drift apart. The matches! arm compiles to exactly what it did before.

Auditing the other callers of the same function and reporting them (symbol/get.rs passes byte-string literals; the three in descriptors.rs and the class registry already copy) is what makes "these two were the only ones getting it wrong" a checked claim rather than a hope.

The test is the right instrument, and I verified it bites

a_computed_key_buffer_method_never_captures_a_temporary asserts pointer identity with the static literal — deterministic, no GC timing required. Restoring the borrow-from-local fails it:

the computed-key arm must capture the 'static literal — anything else is storage the caller owns and the closure outlives

That matters because the runtime symptom is not deterministic, and I proved that the hard way. My first attempt to reproduce the crash ran the two gap fixtures under PERRY_GC_ZEAL=1 PERRY_GC_ZEAL_ALLOC_KB=0 and saw 20/20 nonzero exits on main — then 20/20 on this branch too, which reads as "the fix doesn't work". Both were exit 70: #7604's verdict saying "THIS RUN EXERCISED NOTHING WORTH TRUSTING… not one back-edge poll was reached". I nearly reported both the bug and the fix wrongly from a misread exit code, and the instrument is the only reason I didn't. A pointer-identity assertion has no such failure mode.

cargo test -p perry-runtime --lib: 1988 passed, 0 failed. Gates 21/21.

One note: both fixtures are absent from gap_snapshot.json, meaning both are expected to pass — so this was a live conformance failure, not a known-red entry.