Carry the page tail from where the last page actually ended - #159
Open
Janzert wants to merge 1 commit into
Open
Conversation
Scans go a page at a time, with the last N - 1 bytes of the previous page placed in front of the next so a signature lying across the boundary is still found. The offset that tail was taken from assumed the previous page had been a full 4 KiB, which is true of 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. So a range ending part way into a page asked for more bytes than the page held and panicked in `copy_from_slice` -- and `mono::Module::attach` scans `(mono_assembly_foreach, 0x100)`, which straddles a page boundary whenever that symbol lands within 256 bytes of one. A range starting part way into a page carried bytes that had never been read, and missed a signature across its first boundary without saying anything. Track how much the last page actually held, and carry that much -- clamped to the head, which is all the room there is. It also removes the case distinction in building the slice to scan: how far in front of the page the scan starts is now a number rather than a branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Follow-up to #158. Same function, different bug. This one is not a soundness
issue, it's a panic that a given mono build will either hit on every attach,
or always pass without a problem.
Scans go a page at a time, with the last
N - 1bytes of the previous pageplaced in front of the next so a signature lying across the boundary is still
found. The offset that tail is taken from assumes the previous page was a full
4 KiB. That holds for every page but two: the first page of a range that does
not start on a page boundary, and the last page of one that does not end on
one.
held, and panics in
copy_from_slice.and misses a signature across its first boundary silently.
Which of those anything actually reaches
Every scan asr performs is over a module range or a memory range, except for a
handful of the form
(some symbol, 0x100)— five inmono::Module::attach,one per pointer-size/format arm, plus a couple in il2cpp and in the PS1/PS2
retroarch backends.
Module and memory ranges are page-aligned at both ends, so they reach neither
bug, which is why this has not been panicking everywhere. The
(symbol, 0x100)scans are aligned at neither end. Those reach the panic, whenever the symbol
lands within 256 bytes of a page boundary.
It is worth being precise about the shape of that risk, because "luck" is
misleading. Module bases are page-aligned, so the symbol's offset within a page
is
RVA & 0xfff— a property of the shippedmono-2.0-bdwgc.dlland nothingelse. Two builds I could measure:
0x3900x790Separate launches of the same build carry the same offset, so the dice are
rolled once per mono build, not once per launch. Functions are 16-byte
aligned, and 15 of the 256 aligned slots in a page put the scan across
a boundary (
0xf00exactly is safe — the range ends on the boundary ratherthan crossing it), so roughly 6% of builds. A bad slot assignment will not
cause an intermittent fault; it will panic on attach every time.
Nothing within asr reaches the silent miss. It needs a range that begins part
way into a page and then runs a full page further, and asr never builds one
— its unaligned scans are 0x100 or 0x200 bytes long, which hits the panic first
if it straddles anything.
The fix
Track how much the previous page actually held and carry that much, clamped to
the head, which is all the room there is. Positions coming back from the scan
are then relative to the start of the carried bytes rather than to a page that
may not have been full.
It also removes the case distinction in building the slice to scan: how far in
front of the page the scan starts is now a number rather than a branch. No API
change needed.
Tests
I have two regression tests for this — one per bullet above, driving
scan_process_rangeover a stubbed process so the ranges can be builtdeliberately unaligned. Both tests fail on master and pass with this change.
Once the overall testing strategy has landed, I can supply them if desired.
🤖 Code and comments initially written with Claude Code