perf(intel): refuse gap candidates the exception directory places inside a function

[r0ny123]

A 64-bit PE carries one RUNTIME_FUNCTION record per function, each naming the extent the unwinder needs. Candidate discovery already reads that table to seed function starts — and never consults it again. So the gap scan stayed free to carve extra entries out of addresses the image had already declared to belong to a routine that starts earlier.

Those are precisely the addresses the gap scan reaches: no reference and no prologue claims them, which is why they survive to it.

Root cause, measured before anything was written

Over three 64-bit PE cells of a built Rust corpus, of the false positives the gap scan alone booked, 734 of 743 (98.8%) sit strictly inside a declared .pdata extent. The two populations separate exactly, not statistically:

• 0 of the 878 true positives the gap scan alone booked are interior to a declared extent.
• 0 of the 5,114 truth functions on those cells are interior to a declared extent.

The fix

USE_PE_X64_PDATA_INTERIOR_GAPS, on by default. A gap candidate the exception directory places inside a function is refused, and the scan resumes at that function's end rather than one byte on.

Four conditions keep it from overreaching. Three were needed from the start; the fourth came from a corpus that caught the change being wrong.

• A chained record (UNW_FLAG_CHAININFO) describes a fragment of another function, so its own first byte is interior too. A primary record's first byte is the entry and stays bookable.
• A primary extent only suppresses once the function it names has actually been recovered.
• Each extent is bounds-checked before it is believed: end above start, end within the image, span under _PDATA_MAX_FUNCTION_SIZE, unwind pointer DWORD-aligned, and its UNWIND_INFO first byte one of the eight legal values. These are the bounds _readExceptionRecord already applies when it has to find the table itself.
• Extents are recorded only from a table the image declares. That one is the whole difference between a win and a regression — see below.

Extents are deliberately not merged: functions are laid out end-to-start, and merging adjacent records would collapse .text into a handful of spans in which every address but the first reads as interior.

The regression this shipped with, and what fixed it

The first version trusted the table however it was obtained. On built corpora that was invisible. On a 57-sample malware corpus it cost 1,274 functions on one memory dump, 254 of which a structural reading calls real.

The engine reads the exception table two ways and they are not equally trustworthy. When the image declares a section table, the directory's location is read from it. When it does not — normal for a memory dump, whose mapped bytes survive while its header does not — the table has to be found, by scanning for a run of entries that validate as RUNTIME_FUNCTION records. That reconstruction was already used to seed candidates, where a wrong entry costs one bad address. Extending it to suppression made a wrong entry cost every gap-only function inside whatever range it named.

	malware dumps, n=57
trusting a carved table	−1,274 functions on one sample
declared tables only	−1 function, −1 false positive

The narrowing is free on every image that has a section header — built Rust is byte-identical across it — and is the whole of the regression on the images that do not. The direct control is a 56-sample headerless ByteWeight variant, the same PE64 binaries with their headers stripped: bit-identical on both sides, because the only table available there is a carved one and a carved one now suppresses nothing.

The general form is worth keeping: seeding is cheap and self-correcting; suppression is expensive and silent. A wrong seed is one false positive later passes may absorb. A wrong suppression removes a region and nothing reports that it was removed. A source good enough for one is not automatically good enough for the other.

The one remaining loss is a defect in the oracle

0x14001161b on an x64 dump. The image's own .pdata declares a single function [0x140011424, 0x140011662); the corpus' manually annotated truth puts a second function inside it. The contested address is unaligned where 87.8% of that sample's truth is 16-aligned, has zero inbound references, and falls through without returning — eleven instructions ending in a mov. The function it sits inside is referenced, 133 instructions long, and ends in ret.

That is a tail block annotated as a function. It is stated rather than argued away, because a single contested address is exactly where the temptation to fit the measurement to the change is strongest.

What it moves

corpus	n	PPV	TPR	Δ FP	Δ TP
ByteWeight MSVC PE64	68	99.080 → 99.869	99.838 → 99.851	−1,220	+48
Built Rust	24	75.915 → 79.477	97.496 → 97.584	−2,052	+48
Built Go	47	94.036 → 94.342	unchanged	−538	0
malware dumps	57	92.639 → 92.645	98.561 → 98.552	−1	−1 (above)

Recall rises on three of the four. That was not the intent — the rule only refuses candidates — and the mechanism is worth naming: booking an interior address is not merely a wrong entry, it starts an analysis that runs past the end of the routine the address sits inside and absorbs the small aligned functions after it. All eight functions recovered on the cell examined sit 11 to 79 bytes past a declared extent's end.

Controls

Every corpus that does not carry a 64-bit PE exception directory is bit-identical:

corpus	n	result
Built C/C++ ELF, x86-64	140	bit-identical — 93.740 / 96.968, 113,438 TP, 14,123 FP both sides
ByteWeight MSVC PE32	68	bit-identical — 92.041 / 97.872
ByteWeight MSVC PE64, headerless dumps	56	bit-identical — 98.874 / 99.811, 106,403 TP, 2,052 FP both sides
Built Rust, linux-gnu and windows-gnu-x86	16 of 24	bit-identical

The x86-64 ELF corpus is the control that counts, because it runs the same backend and the same gap scan on a container that declares no exception directory. ARM64 corpora prove nothing here — AArch64 has its own candidate manager and never reaches this code.

The 32-bit result has a structural explanation rather than a lucky one: 0 of 68 32-bit PEs declare an exception directory, against 68 of 68 of the 64-bit ones.

Bug-class sweep

The class is a declared function extent the engine reads for candidates but never for suppression.

site	state
intel, PE x64 .pdata, declared table	fixed here
intel, PE x64 .pdata, carved from a headerless dump	deliberately not used for suppression — see above
intel, ELF .eh_frame	covered by USE_ELF_FDE_INTERIOR_GAPS in #300
aarch64, PE ARM64 .pdata	not fixed here — the extent is reconstructible, but there is no ARM64 PE anywhere in the 305 operator-supplied samples available, and this PR is itself the argument against shipping an unmeasured suppression rule
CIL, Dalvik	cannot occur; neither backend gap-scans, the base nextGapCandidate raises
Mach-O LC_FUNCTION_STARTS	names starts, not extents; nothing to suppress with

Review findings addressed

An adversarial review of this diff found seven items. The two that mattered:

• The walk-back in declaredExceptionRangeContaining was wrong, and its docstring justified it with a false claim — sorting by start does not order the ends, so "the previous record ends before this address" says nothing about the one before that. Two short extents in front of a long one ended the walk on top of the extent that actually covered the address. Replaced with a running maximum of reach. It failed open, so it could only cost precision, and it is unreachable from a conforming table — but the reasoning was wrong and written down as if it were not.
• The recovered-function guard had no coverage. Its test used a record with End below Begin, so no extent was recorded at all and the assertion held for an unrelated reason; deleting the guard entirely left the suite green. Same for the image-bound check, shadowed by the span cap.

All three guards are now mutation-tested: deleting any one of them makes a test fail. Also from that review — a PE check, since getSections() yields for ELF and Mach-O and a section merely named .pdata should not be read as an exception directory; the DWORD-alignment test the carved path already required; and a note on the extent list's memory (~29 MB at 200,000 functions, uncapped on purpose, because a cap would silently stop suppressing on exactly the largest images).

Tests

tests/testPdataInteriorGaps.py, 30 cases over a minimal spec-valid PE32+ x86-64 image with .text, .pdata and .xdata, driven end to end, plus the lookup and the extent predicate on their own where every branch can be reached with a chosen table. A headerless fixture with a stated bitness drives the carve path.

The end-to-end fixture's island is mov eax,1; ret rather than a prologue shape on purpose: the first version used push rbp; mov rbp,rsp, and the prologue scan booked it before the gap scan ever saw it, so the test would have passed for the wrong reason.

tests/testPdataExtraction.py gains one method on its MockBinaryInfo — the double now answers what container it stands for, which is what the new PE check asks.

Validation

ruff check / ruff format --check pass, full suite green on this branch with current master merged in (1913 passed, 1 skipped, 2591 subtests), diff coverage 100% with 0 missing lines.

Every figure above was taken on the change as proposed, not carried over from the run that motivated it. The built-Rust cells were re-measured after the review fixes and came back identical to the run that motivated them (79.477 / 97.584, 33,052 TP, 7,397 FP), so those fixes are behaviour-neutral on real images as well as bit-exact in intent.

Branch shape

Rebased onto current master (395c88d) as a single commit, dropping the merge and the fix(ci) commit that #302 made redundant.

Content-neutral: the tree is 0fb222b, byte for byte what the merge-based version resolved to, so the measurements above stand and nothing needed re-running. The one conflict — the SmdaConfig.py clash with #305, both appending an option block to the same region — is resolved by keeping both blocks, and the resulting option set is exactly the union of the two sides. ruff check, ruff format --check and ty 0.0.74 clean; the full suite was 1913 passed / 1 skipped / 2591 subtests on this identical tree.

Scope note: the is_pe guard also gates seeding

Putting this on the record, since it is a real widening of scope that the title does not suggest.

The is_pe guard does not only gate suppression — it also stops .pdata seeding on a non-PE image. Before this, a 64-bit ELF carrying a section named .pdata would seed candidates from those bytes; now it will not.

I think that is right on its own terms: RUNTIME_FUNCTION is a PE construct, and reading arbitrary bytes out of a same-named ELF section as unwind records is not sound to begin with. But it is a change to candidate discovery living inside a PR about refusing gap candidates, so it should be a decision rather than a side effect. No bundled non-PE image carries such a section, so it is inert on the current fixtures either way. Happy to split it out if you would rather it were its own change.

Relationship to #299 and #300

This change is also sitting inside #299 and #300. Both of those were opened from branches that had my fork's master merged into them repeatedly, so their diffs are much wider than their titles suggest and they overlap each other as well.

This PR is the same change on its own, so it can be reviewed and measured for what it actually is. If you would rather take it through one of those, close this one; if you take this one, the matching hunks fall out of theirs on merge.

[danielplohmann]

Reviewed, and I'd like to take this — but it needs a rebase first: GitHub now reports it CONFLICTING.

The conflict

One file, src/smda/SmdaConfig.py, and it is purely textual. #305 landed as 59b6e11 and appended USE_READYTORUN_NATIVE_ROUTING to the same region this appends USE_PE_X64_PDATA_INTERIOR_GAPS to. The resolution is to keep both blocks — exactly what you predicted in #305's "Notes for merging". Nothing here reads or writes the other's option.

I resolved it locally that way to review the branch, and everything below was measured on that resolution, so the rebase should be mechanical.

What I verified, so you know the review is done and only the rebase is outstanding

Scored tests/rust_pe_gnu_xored against its own COFF symbol table (a mingw build that retains 4,818 symbols), before and after:

	before	after	delta
TP	2047	2047	0 — no true positive lost
FP	342	154	−188
PPV	85.684%	93.003%	+7.32pp

Every one of the 188 suppressed addresses is a false positive on that fixture. That is a stronger result than the +3.56pp your built-Rust table reports, on the one bundled fixture that moves.

Your controls hold on the bundled set too: mirai_x64_xored (x86-64 ELF), cutwail_xored and rust_pe_msvc_i686_xored (32-bit PE) all come back bit-identical.

I unit-tested declaredExceptionRangeContaining against the adversarial shape its own docstring names — two short records sorted in front of a long one — and the running-maximum reach array handles it correctly (0x1500 resolves to (0x1040, 0x2000) rather than stopping on the short records). Nested extents resolve to the innermost, which is the conservative direction. Chained-vs-primary at the extent's first byte behaves as documented.

And the regression guard is structural rather than incidental: instrumenting _admitExceptionRecord shows 1,666 records admitting with declared=True on an intact PE, while _carveExceptionRecords passes declared=False and contributes nothing to _pdata_ranges. The distinction you drew after the −1,274-function regression is doing real work, and it is worth keeping as a stated principle — seeding is cheap and self-correcting, suppression is expensive and silent — beyond this PR.

Full suite on the rebase: 1903 passed, 2 skipped, 2588 subtests. 25 of the 41 new tests fail against master, so they are non-vacuous.

One scope note, not a blocker

The is_pe guard also stops seeding from a non-PE .pdata section, not only suppression. Before this, a 64-bit ELF carrying a section named .pdata would seed candidates from those bytes; now it will not. That is defensible on its own terms — RUNTIME_FUNCTION is a PE construct — but it is a change to candidate discovery inside a PR about refusing gap candidates, and the title does not suggest it. No bundled non-PE image carries such a section, so it is inert on the current fixtures. Mentioning it so it is on the record rather than asking you to change it.

Also affected

#312 stacks on this and hits the same SmdaConfig.py conflict, so it will want the same treatment. #299 and #300 are conflicting too — #300 on SmdaConfig.py, #299 on SmdaConfig.py plus src/smda/common/BinaryInfo.py (that one against #302's annotations, which are now in master as 7f892b9).

#310 and #311 still merge cleanly, so those are not waiting on anything.

No rush on any of it — and thanks for the care in these, the self-caught .pdata regression writeup in particular was more useful to read than most postmortems.

[danielplohmann]

Update, and an extra thing for the same rebase pass.

#310 is merged (7d01480), and #311 is reviewed and wanted — but it now conflicts, for a different reason than #309 does, and the reason is on our side rather than yours.

The squash artifact on #311 and #312

This repository squash-merges, so #310 landed as one new commit with a new SHA. #311 still carries #310's three original commits (9233624, fa31fe1, e8626b7), and git no longer recognises them as ancestors of master — so they collide with the squashed version in src/smda/aarch64/FunctionCandidateManager.py. Nothing is wrong with the branch; it is the shape a stacked PR takes after its parent is squashed.

The resolution is to drop those three commits. #311's own ten apply cleanly onto current master — I verified by cherry-picking exactly this set and it went through without a conflict:

1ce9b87  fix(aarch64): read the live function set, not only the candidate snapshot
cdb3fab  test(aarch64): pin the gap-run terminator asymmetry rather than remove it
5a4c824  test(aarch64): keep the reasoning for the terminator asymmetry, not the numbers
c7d2b92  perf(aarch64): a hoisted guard is a function's first instruction, not a stray tail
b5cb3e4  test(aarch64): bring the hoisted-guard tests back to the density of their neighbours
bc455c3  perf(aarch64): skip the address scan on an image whose metadata already named its functions
b42b2b8  fix(aarch64): a coverage ratio needs a sample before it can be read
2cfef31  perf(aarch64): turn on the Mach-O address-materialization scan
ac2cb4a  fix(aarch64): read the coverage gate from the image, not from the candidate set
1ac1ab8  test(aarch64): move the pinned fixture baselines this series changes

The fix(ci) commit at the tip can go too — #302 is in master as 7f892b9.

#312 will hit both problems: the same three duplicated commits, plus #309's SmdaConfig.py collision, plus #311's ten once those land. Taking #309 → #311 → #312 in that order leaves #312 as the single commit you described.

What #311 verified as, so you know only the rebase is outstanding

Scored the bundled 11-sample ARM64 Mach-O corpus against LC_FUNCTION_STARTS, before and after:

	before	after	delta
TP	1787	1818	+31
FP	1112	1107	−5
TPR	86.916%	88.424%	+1.51pp

No cell loses recall. osx.frostyferret gains 11, which lines up with the +8 you attribute to reading the live function set and the +2 to turning the Mach-O scan on; osx.gimmick +11, rustypages +4, lockbit +3, poseidonstealer +2.

The hoisted-guard change shows up on aarch64_static_xored too, which keeps 278 functions while one start moves four bytes earlier. Disassembling around it:

0x40df2c  nop                    <- inter-function padding
0x40df30  cbz x14, #0x40dfc8     <- the new entry
0x40df34  add x4, x1, x2         <- what was booked before

The padding in front settles it — 0x40df30 is the boundary, and the old guard was booking the function one word inside its own body. That is a corrected entry rather than a moved one, and it is the clearest demonstration of your point that the claim in that comment was just false.

I also instrumented the coverage gate across all 13 bundled AArch64 images. It fires exactly once: twelve fall below _METADATA_MIN_CALL_TARGETS and are left to the scan — BlueNoroff_469fd8a280e8 resolves four bl targets in the whole binary, which is precisely the small-sample case your comment cites — and Turtle_5f9cd91d8d1d clears the threshold at 98.3% (1243 of 1265) and skips it, with output bit-identical either way. So on bundled data the gate is a work saving with no accuracy consequence, which is the right way round for something whose two errors are asymmetric.

Full suite on the rebase: 1917 passed, 2 skipped, 2593 subtests. Eight tests fail against pre-#311 source, plus testAArch64MetadataCoverageGate.py erroring on import, so the set is non-vacuous.

Two observations on #311, neither blocking

It flips a default. USE_MACHO_ADDRESS_REF_CANDIDATES goes False → True. That is justified by the gate now deciding per image rather than per container, and the bundled evidence supports it — but it is worth its own line in the changelog rather than arriving inside a follow-ups PR.

Six changes under one title. Having read it, the bundling is defensible: 1, 3 and 5 all feed the same two scans and 2 is comment-only. Splitting them now would cost more than it buys. Mentioning it because the measured effects are per-change while the merge is atomic, so if one of them ever needs reverting it will not come apart cleanly.

Nothing here needs a reply — the rebase is the only thing between these and merging, and #310's result is already in.

[r0ny123]

Thanks — that is a much better measurement than the one I opened with, and the rust_pe_gnu_xored scoring against its own COFF table is the control I should have built in the first place.

All five of my open PRs are mergeable again: #309, #311, #312, #299 and #300.

I merged master in rather than rebasing, so nothing force-pushes out from under the review you have already done — the resulting tree is the resolution you describe, keep both option blocks, and the option set is exactly the union of the two sides.

One thing worth passing on, because it bit three of the five. The merge silently reverted #318 the first time. These branches had been carrying #302's commit so their own CI would be readable while master was red. #302 dropped the -> bool on smda_instruction_matches_capstone; #318 put it back with the bool() wrap. Master's net change at that line is therefore zero, so the three-way merge saw only my side as having changed it and took the deletion — quietly undoing your fix in a file none of these PRs has any business touching. Caught it by diffing each merged tree against master and asking why a file outside the PR's scope still differed. All three now leave that file alone.

I checked the general form of it too rather than just the one instance: for every file each branch touches, every line master has added since 572acd7 is present in the merged tree. Zero missing on all five.

#311 needed a merge after all. It was clean when you looked, but #310 landing changed that — #311 stacks on #310, so its own commits are now on both sides and four regions conflicted. All four resolve to the branch's side, which I checked rather than assumed: is_conditional_branch appears twice in both 572acd7 and master, so master added nothing there and the branch's removal of one occurrence is its own intended change.

On the is_pe scope note — you are right, and I would rather it were a decision than a side effect. I have written it into the PR body so it is on the record: the guard also stops seeding from a non-PE .pdata, not just suppression, so a 64-bit ELF with a section of that name no longer seeds candidates from those bytes. I think that is correct on the merits, since RUNTIME_FUNCTION is a PE construct and reading those bytes as unwind records was never sound, but it is a candidate-discovery change inside a PR about refusing gap candidates. Happy to split it into its own change if you would prefer to take the two separately.

Full suite green on each merged tree, with ruff check, ruff format --check and ty 0.0.74 clean: #309 1913 passed, #311 1918, #312 1959, #300 2014, #299 2047 — 1 skipped in each.

And thank you for unit-testing declaredExceptionRangeContaining against the sorted-short-records shape. That is the case I was least sure of, and I would not have known the running-maximum reach array actually held up under it without you checking.

[r0ny123]

Your squash diagnosis is exactly right, and it is what I hit — #311 carried #310's three pre-squash commits, so they collided with the squashed 7d01480 in the candidate manager.

I resolved it by merging master in rather than rebasing, so nothing force-pushes out from under a review you have already done. The result is the same tree you verified. I cherry-picked your exact ten onto current master and compared:

your rebase       tree 38b27ba6eaea1d85f65f11d19d9fff25b6f604f1
#311 as it stands tree 38b27ba6eaea1d85f65f11d19d9fff25b6f604f1

Byte-identical. So squash-merging #311 as it is now lands precisely the content you measured — the rebase would change the history shape without changing a single byte of the outcome. Given that, I would rather not force-push over a reviewed branch for cosmetics, but say the word and I will flatten it to the linear ten if you prefer that shape in the log.

The fix(ci) commit needs no action either — it dropped out by itself when master came in, since #302 is 7f892b9 now. None of these branches carries it any more.

#312 checked the same way. I simulated your order: applied #309 onto master-plus-#311, then diffed #312 against that. It reduces to exactly its own content — five files, the ARM64 option, the aarch64 gap-scan block, the _pdata_ranges hoist into common/ with the matching removal from intel/, and its test file. The single commit you described.

So #309 → #311 → #312 works as you laid it out, with no rebase pass needed anywhere.

On the default flip — agreed, that deserves to be visible at release rather than buried in a follow-ups PR. I have put a ready-to-lift paragraph in #311's body under "Changelog note". I deliberately did not touch CHANGELOG.md itself: its entries are per released version and there is no unreleased section, so adding one means choosing a version number, and that is yours.

On six changes under one title — fair, and I will take the point about revert granularity. The risk is real precisely because the measurements are per-change while the merge is atomic; if one of them does need backing out later, it will not come apart cleanly and someone will be reconstructing it by hand. Not worth splitting now, but worth me not repeating on the next one.

[r0ny123]

All three are now flat on current master, so the whole stack is in the shape you asked for rather than just #311:

	commits	head	tree
#309	1	7d028fc	0fb222b
#311	10	7bc8107	38b27ba
#312	14	cae175f	0b2ce4d

Every one is content-neutral — each tree is byte for byte what the merge-based version resolved to, so nothing above needs re-measuring. #312 had accumulated more than the two of us spotted: five merge commits, the three pre-squash copies of #310, and three duplicate fix(ci) commits. All gone; its 14 are #309's one, #311's ten and its own three, and it reduces to those three once the first two land.

One thing I should flag rather than let you find it. Rewriting these turned up a leaked reference in two commit messages: a #147 that meant my fork's issue and resolves here to an unrelated actions/checkout bump, plus two dangling "the issue" phrases pointing at fork issues no reader here can reach. Reworded to stand on their own — no content change, trees unaffected. My fault for not catching it before opening these; I have checked the rest of the messages on all three and they are clean now.

#299 and #300 still carry fork PR numbers in their merge-commit subjects (Merge pull request #132 from r0ny123/... and similar), which link to unrelated PRs here. I have left those alone deliberately — flattening two branches you have already reviewed is a bigger rewrite than the problem warrants, and squash-merging drops those subjects anyway. Say the word if you would rather they were cleaned up.