feat(common): refuse a candidate the image declares interior, not only a gap one - #327
feat(common): refuse a candidate the image declares interior, not only a gap one#327r0ny123 wants to merge 3 commits into
Conversation
634a8f9 to
a113ae9
Compare
|
Reviewed against the source rather than the figures, since the figures are not reproducible here. The guards each have their own case and each case fails without its guard — the PLT exemption, the recovered-owner test, the recovered-extent test, the range-start exclusion and the flag control are eight unit cases, and One thing in the docstring does not hold, and it is the sentence the next person will lean on
The hook sits immediately after Confirmed on the two refusals I could provoke: both report That does not make the rule wrong — it is the same evidence, and your 0 true positives lost across 212 ELF cells is what says it costs nothing. But the reason stated is not the reason it works, and the residual exposure is the one the sentence denies: a real entry with no FDE of its own, sitting in a hole inside another routine's declared range and recovered envelope, loses whatever reference only its bytes carry — the same shape as the three functions the extent guard rescued, one scale down. Anything carrying its own FDE is safe, since Worth rewriting to say what the population is and that the measurement is what bounds the risk. A sentence, not a change of code. What the bundled fixtures say about this rule: nothingInstrumented
Exactly as your test's docstring says, and I am not asking for a fixture that changes it — by the standard we just settled on #322 item 2, the mechanism is pinned in-repo and the magnitude rests on your corpora. It does mean the v4.7.0 changelog entry has to say so: under the provenance rule from #323 the −683 and −77 go in as measured on your corpora and not reproduced here, the way v4.5.1 marked #310's and #312's figures. SequencingI am holding this rather than merging it now, because #329 is stacked on it and has a blocking finding of its own — it breaks |
|
You are right, and it is worse than a loose sentence — it stated the opposite of what the code does. Confirmed against the source here: The paragraph now says what the population is and what bounds the risk:
The magnitude stays out of the docstring deliberately — an absolute false-positive count is a property of every other rule in the engine on the day it was taken, and belongs in the entry that dates it. The invariant is what keeps its meaning in a comment. One more, from re-reading the diff after yours
On the changelogAgreed, and it is yours to write at release: under the provenance rule the −683 and −77 go in as measured on corpora not bundled here, the way v4.5.1 marked the earlier two. Nothing in this PR's tests rests on them — the bundled ELF fixtures produce 0 refusals, exactly as the test docstring says. Sequencing#329's blocking finding is fixed and pushed; the shared-helper change lands there, on top of this. Master is merged into both rather than rebased — I would rather not force-push over a branch you have already reviewed, so taking the rebase at merge is yours. Full suite green on this tip: 2,119 passed, 1 skipped, 2,596 subtests. |
…y a gap one The .eh_frame rule added in danielplohmann#300 refuses a candidate that opens strictly inside a range the image declares, and it is reached only from the gap scan: it tests self.gap_pointer, so candidates from the prologue scan, from branch targets and from the tailcall paths never meet it. On the 72-cell AArch64 ELF corpus 2,245 of the 2,484 false positives still standing are interior to a declared range, and none of them is a gap candidate. Ask the same question where analysis is about to begin on a candidate from any source. Both of the gap rule's guards apply unchanged: a PLT is exempt because the whole table sits under one FDE, and the range's own start has to be a recovered function because an FDE can begin in the alignment padding ahead of its own. A third guard is new, and the corpus is what found it. A declared range can reach past everything its function's control flow arrives at, and refusing an address out there discards bytes nothing else claims along with any reference only those bytes carry. Without it the AArch64 corpus loses three functions, each the sole target of a call sitting in exactly that unreached tail. Requiring the owner's own recovered extent to surround the address costs 163 of the refusals and returns all three. Measured against compiler symbol tables, no corpus losing a true positive: 72 AArch64 ELF cells PPV 95.994 -> 97.063 -683 FP, TP and FN identical 140 built C/C++ ELF PPV 98.903 -> 98.969 -77 FP, TP and FN identical The 120 MinGW PE cells, 23 Go cells, 11 ARM64 Mach-O cells and all 57 malpedia dumps are bit-identical, which is the control that it reaches only images carrying an .eh_frame. Rust is bit-identical too, and not because the rule is inert there: its images decode their ranges and 25 of 26 false positives on the first cell are interior to one, but the guards decline all of them. Analysis is slightly faster, because a refused candidate is one nothing then analyses. Also moves a stray unittest.main() in the test file, which sat above a test class and so left that class undefined when the file is run directly.
The docstring said that inside the owner's recovered extent "the owner already accounts for the address, so nothing is lost". It does not. The rule is consulted immediately after the collision check, which tests membership in a byte-level code map, so every address that reaches it is one the owner's analysis never covered; function_borders holds only the extremes of that code, not its coverage. That leaves a residual exposure the old sentence denied: a real entry carrying no FDE of its own, sitting in a hole inside a neighbour's declared range and recovered envelope. Nothing in the format rules that shape out -- an entry with its own FDE is never interior to it, since a range's own start is excluded -- so what bounds it is the measurement, not the structure. Say that instead.
analysedFixture() patches declaredInteriorOwner on the class and restores it in a finally, but the sample write and the disassembler construction sat outside the try. Either raising would leave the claim installed for every later test in the process, turning one failure into a cascade that does not name its cause. Put everything after the patch inside the try.
891f046 to
a4c1513
Compare
…e too Third of the three rules that read declared evidence only at the gap pointer, and the last one with anything left to reach. danielplohmann#327 widened the .eh_frame arm; this does the same for the PE exception directory, under the guards that arm established: the record's own function has to be recovered, and its recovered extent has to surround the address. A fragment record is not the shortcut it is in the gap scan. Its own start is not the function covering the address, so it fails the recovered-owner test and declines rather than refusing, which is the conservative reading at a point where a better one is available. It is worth much less than the .eh_frame arm, which is why it is a change of its own rather than part of danielplohmann#327: 120 MinGW PE x64 cells -47 FP at identical TP and FN 2 Rust windows-gnu-x64 -2 FP at identical TP and FN Nothing else moves. The 140 x86-64 ELF cells, 72 AArch64 ELF cells, 23 Go cells, 11 ARM64 Mach-O cells, the 32-bit and ELF Rust cells and all 57 malpedia dumps are bit-identical, and only PE x64 cells differ in any corpus. The dumps are the reading worth keeping: none of the 57 declares an exception directory at all, so this evidence does not reach a mapped image, which is the corpus this project cares most about.
|
Rebased over #329 is rebased onto this tip, so it still stacks cleanly and has to merge after it. |
2 participants
Closes #324.
USE_ELF_FDE_INTERIOR_GAPSrefuses a candidate that opens strictly inside a range the image's own.eh_framedeclares. It is reached only from the gap scan — it testsself.gap_pointer— so candidates from the prologue scan, from branch targets and from the tailcall paths never meet it. On the 72-cell AArch64 ELF corpus, 2,245 of the 2,484 false positives still standing after #300 are interior to a declared range, and not one of them is a gap candidate: 1,213 come from the initial seeding scans and 1,001 carry a call reference.This asks the same question at the point analysis is about to begin on a candidate, whatever seeded it. Nothing new is read from the image and no heuristic is added; the evidence and the rule are the ones already shipped.
The guards
Both of the gap rule's conditions apply unchanged. A PLT is exempt, because the whole table sits under one FDE and the range test would read every stub after the first as interior to it. And the range's own start has to be a recovered function, because an FDE can begin in the alignment padding ahead of its function, which leaves the real entry a few bytes in interior to nothing.
A third is new, and the corpus is what found it. A declared range can reach past everything its function's control flow actually arrives at. Refusing an address out there discards bytes nothing else claims — along with any reference only those bytes carry. Without this guard the AArch64 corpus loses three true positives, and all three are the same shape: a real function whose only incoming reference sits in the unreached tail of another function's declared range. In
brotli_gcc-arm64_O2-staticthe entry point's FDE is[0x401e40, 0x401e7c), its control flow stops before0x401e78, and theblat that address is the only thing naming the real function at0x400600.Requiring the owner's own recovered extent to surround the address costs 163 of the refusals and returns all three. It is also most of why Rust comes out unchanged.
Measured
Against compiler symbol tables,
mastere9dfe5f→ this branch, both trees run back to back on the same machine. Re-measured after the rebase onto #325; the figures are unchanged, which is expected since that PR's one behaviour change is PE-only and this rule is ELF-only. No corpus loses a true positive.LC_FUNCTION_STARTS).fnmaptruth)Micro (pooled) figures; TP and FN are identical in every row. The four bit-identical rows are the control, not filler: 120 PE cells, 23 Go, 11 Mach-O and 57 memory dumps carry no
.eh_frame, so any movement there would have meant the rule firing where it has no business firing.Rust is bit-identical for a reason, not because the rule is inert there. Its images decode their ranges — 740 FDEs on the first cell — and 25 of that cell's 26 false positives are interior to one. The guards decline all of them: over its first four cells, 78 of 94 on the owner's recovered extent and the other 12 on the owner not being recovered.
The rule never refuses an address the image names. Across the four densest AArch64 cells it refuses 4,070 addresses and every one is inferred-only — not a symbol, not an exception record, not language metadata, not a gap or tailcall candidate.
Analysis is slightly faster, because a refused candidate is one nothing then analyses:
zlib_gcc-arm64_O2-static1.26s → 1.19s,lua_gcc-arm64_O2-static2.04s → 1.92s (median of 3).Tests
Nine new cases in
tests/testFdeInteriorGaps.py. Eight drivedeclaredInteriorOwnerdirectly, one condition each, and the ninth covers the wiring: it claims one address the fixture does recover and asserts analysis then declines it. Each was checked to fail when the guard it describes is removed, and only that one — dropping the extent guard fails two, the PLT exemption one, the recovered-owner condition one, and removing the call site fails the wiring case.The bundled fixtures reach the gap scan's own refusal or the collision check before this arm sees anything, which is why the wiring case claims an address rather than asserting over what the fixture happens to drop. An earlier version of it asserted over the recovered set and passed with the feature removed; that is why it is written this way.
This file also had
unittest.main()sitting above a test class, so running it directly never definedFdeInteriorGapRuleAArch64Test. Moved to the end.python -m pytest tests/→ 2,069 passed, 1 skipped, 2,593 subtestsruff check .andruff format --check .→ cleanmake typecheck→ exit 0, 0 error-level diagnosticsNo bundled fixture baseline moves.
One sibling, deliberately left out
Three rules read declared evidence at the gap pointer only: the
.pdataexception range,USE_LSDA_LANDING_PADS, and the one widened here. The landing-pad rule has nothing left to reach — of 302 false positives still standing on four AArch64 cells, none is a declared landing pad. The.pdatarule does: 6 of 74 remaining false positives on three MinGW PE cells are inside a declared range.That one is left for its own change. It moves PE, which is exactly the control that makes this one safe to read, and it needs its own measurement over the PE corpora rather than riding on these numbers. Happy to open an issue for it, or fold it in here if you would rather see them together.
Corrected after review
The docstring's claim about what the rule can reach was wrong. It said that inside the owner's recovered extent "the owner already accounts for the address, so nothing is lost". The rule is consulted immediately after the collision check, which tests membership in
code_map— every byte of every recovered instruction — so every address that reaches it is one the owner's analysis never covered, andfunction_bordersis an envelope built from the instruction extremes rather than a coverage map.It now names the population (alignment holes between blocks, an unreached tail, a data island), names the residual exposure (a real entry carrying no FDE of its own, sitting in one of those holes inside a neighbour's declared range), and says that what bounds it is the measurement rather than the structure. The magnitude stays out of the source: an absolute false-positive count is a property of every other rule in the engine on the day it was taken, while the invariant — no true positive lost — is a property of the mechanism.
A test-isolation fix found in the same pass.
analysedFixture()patchesdeclaredInteriorOwneronto the class and restores it in afinally, but the sample write and the disassembler construction sat outside thetry; either raising would leave the claim installed for every later test in the process. Everything after the patch is inside thetrynow.Full suite on the current tip: 2,119 passed, 1 skipped, 2,596 subtests,
ruffclean,make typecheckexit 0.