Fix the signature scanner writing to a freed stack frame - #158
Merged
Conversation
Collaborator
|
That's a really good finding, though, can you remove the tests? Like, there's some other pull requests that also add testing, and it kind of interferes with what you're doing here. Overall, for the testing strategy, I would like to figure out a better strategy long term. So for now, I would want you to just remove the tests. |
Janzert
force-pushed
the
signature-scan-dangling-buffer
branch
from
September 5, 2026 15:38
4a2fdd5 to
b64df45
Compare
`scan_iter` zero-initialises a `Buffer<N>` as a local, takes a `&mut [u8]` over it via `slice::from_raw_parts_mut`, and moves that slice into the `iter::from_fn` closure it returns. Only the slice is moved: the buffer itself remains a local of `scan_iter`, so by the time the returned iterator is first polled the storage the slice points at has been given back, and every poll reads and writes roughly 4 KiB of a dead stack frame. Move the buffer into the closure and take the slice inside each call, where its storage is live for as long as the pointer is used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CryZe
force-pushed
the
signature-scan-dangling-buffer
branch
from
September 5, 2026 15:39
b64df45 to
620f56d
Compare
CryZe
enabled auto-merge (squash)
September 5, 2026 15:39
Contributor
Author
|
Sure, no problem. They were in the last commit by themselves so quite easy to drop. |
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
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.
Signature::scan_iterzero-initialises aBuffer<N>as a local, takes a&mut [u8]over it withslice::from_raw_parts_mut, and moves that sliceinto the
iter::from_fnclosure it returns.Only the slice is moved. The buffer stays a local of
scan_iter, so its frameis gone before the returned iterator is ever polled. Every poll then reads and
writes roughly 4 KiB through a pointer into a dead stack frame — on a scan that
Module::attachruns.That is UB regardless of what it happens to do at runtime: the pointer outlives
the storage it was derived from.
How it was discovered
A test harness replays captured game memory through a chain of twenty delta
snapshots, which puts the caller of the scan about twenty frames deeper than
normal. The scan's 256-byte page read then landed inside a live frame, over
a return address. It only bites when whatever runs below the stale address is
deeper than whatever ran there before. That, and release builds inlining
scan_iteraway entirely, is why it does not normally surface.What decides whether it bites
opt-level, and nothing else I tried:lto = falselto = trueAt
opt-level = 0scan_iteris a real function whose frame is popped. Fromopt-level = 1it is inlined into its caller, so the buffer lands in the framethat then drives the iterator and nothing dangles;
-C llvm-args=--inline-threshold=0does not bring it back at 3. I also checkedone release
wasm32-unknown-unknownbuild of an auto splitter: no standalonescan_iterin the binary, and the frames that drive the scans reserve4224–4256 bytes of shadow stack, which is the buffer sitting in the live frame.
So release builds are very likely fine today — by inlining, not by anything
guaranteeing it. Debug builds are not, and that is what
cargo testuses.The test
Second commit,
tests/signature_scan_buffer.rs. The currentTest (Host)jobruns it as it stands. No CI change needed, and nothing from the crate beyond
the public API.
The runtime side of a read is stubbed. It plants a canary in a local array,
then writes only into the buffer asr asked it to fill — it does not know where
that buffer is. Without the fix in the first commit filling the buffer the
scan handed over writes 256 bytes into the reader's own locals, so the buffer
is a pointer into a frame that has already been given back.
The canary is the whole assertion, because it is the only part that cannot
misfire: locals do not change unless something wrote into them, whichever way
the stack grows and wherever the frames land. Where the buffer sat is reported
with the failure but not asserted on — which sign means trouble depends on the
stack's direction and on how deep the freed frame had been.
So it can miss but not misfire, and for the reason above it only fails at
opt-level = 0. Both are written into the test.The fix
Move the buffer into the closure —
movealready captures it once the outerlet buffer = ...rebinding is removed — and build the slice inside each call,where the storage is live for as long as the pointer is used. One hunk in
src/signature.rs; no behaviour change, no API change.Something else in the same three lines
Not changed here, and mentioned only so it is not a surprise: the tail carried
in front of each page is taken from the offset a full previous page would
have ended at. That holds for every page but two — the first page of a range
that does not start on a boundary, and the last page of one that does not end
on one.
panics in
copy_from_slice.misses a signature across its first boundary silently.
Module and memory ranges are page-aligned at both ends, so they reach neither.
The scans of the form
(some symbol, 0x100)aren't aligned at either end,though, and those reach the panic: five sites in
mono::Module::attach, one perpointer-size/format arm. Module bases being page-aligned, that offset is
RVA & 0xfff— set by the shipped mono binary rather than varying per launch.I measured
0x390on Unity 6000.3 and0x790on 6000.5. With 16-byte functionalignment, 15 of the 256 slots in a page put the scan across a boundary, so
roughly 6% of mono builds; a bad one would panic for every splitter against
every game on that Unity version, every time, rather than failing
intermittently. The silent miss needs a range that begins part way into a page
and then runs a full page further, which asr itself never builds.
This is a pre-existing problem and independent of the dangling reference.
I have a fix and two regression tests and am happy to send them as a separate
pull request.
🤖 Code and comments initially written with Claude Code