feat(common): let the exception directory refuse an interior candidate too - #329
Open
r0ny123 wants to merge 2 commits into
Open
feat(common): let the exception directory refuse an interior candidate too#329r0ny123 wants to merge 2 commits into
r0ny123 wants to merge 2 commits into
Conversation
…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.
…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.
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
1 participant
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.
Closes #328. Stacked on #327 — it extends the helper that PR introduces, so it must merge after it. The diff shown against
masterwill include #327's commit until then; this PR's own commit is2efa621.Third of the three rules named in the #324 sweep, and the last one with anything left to reach. #327 widened the
.eh_framearm to a candidate from any source; this does the same for the PE exception directory, under the guards that arm established.The rule
The same two conditions, unchanged: the
RUNTIME_FUNCTIONrecord's own function has to be recovered, and its recovered extent has to surround the address. The two evidence sources stay format-disjoint exactly as the gap scan's comment already notes —_pdata_rangesis only ever filled from a PE exception directory and the.eh_frameranges decode nothing unless lief reports an ELF — so no address is ever arbitrated between them.One deliberate difference from the gap scan. There, a fragment record is evidence on its own: it says the extent is part of some other function, and the scan needs nothing more. Here it is not taken as that. A fragment's own start is not the function covering the address, so it fails the recovered-owner test and declines rather than refusing. At a point where the better evidence — an actual recovered owner — is available, using the weaker form would be the wrong reading. No fragment record appeared among the interior false positives in the sample either way.
Measured
feat/fde-interior-analysis-gate(#327) → this branch, so the figures isolate this arm.windows-gnu-x64No corpus loses a true positive, and only PE x64 cells differ anywhere — which is the control this arm has to pass, the mirror of #327's ELF-only one.
The part worth reading before merging
On memory dumps this evidence does not exist. None of the 57 malpedia dumps declares an exception directory at all —
_pdata_rangesis empty, so the rule never reaches its first condition. That is the corpus SMDA cares most about, and it is the honest limit of this change.Between that and 47 false positives on one PE corpus, this is a small win, and #328 says so rather than selling it. It is here because it is free of recall risk under guards that are already measured, and because leaving one of three siblings unwidened without a reason is worse than either doing it or declining it on the record. If you would rather close #328 and keep the surface smaller, that is a reasonable call and the numbers are the argument either way.
One thing I had wrong
I filed #328 saying 14 of 1,038 on a 20-cell sample, and scaled that to roughly 80 across 120 cells. The real figure is 47. The sample over-predicted; the table above is the measurement.
I also assumed the Rust corpus was ELF, which is why the two moving cells looked like an anomaly at first. They are
windows-gnu-x64— PE x64, exactly where this applies. The corpus is mixed, and every one of its ELF and 32-bit cells is bit-identical.Tests
Seven cases in
tests/testFdeInteriorGaps.py::DeclaredInteriorOwnerPeTest, one condition each, exercising the arm through a stubbed exception directory so the ELF and PE arms are tested apart. Each was checked to fail when the guard it describes is removed: dropping the arm fails two, dropping its flag check fails one.python -m pytest tests/→ 2,076 passed, 1 skipped, 2,593 subtestsruff check .andruff format --check .→ cleanmake typecheck→ exit 0, 0 error-level diagnostics, no new diagnosticsNo bundled fixture baseline moves.