fix(intel): stop jump-table entry scans at the first unreadable entry - #248
fix(intel): stop jump-table entry scans at the first unreadable entry#248r0ny123 wants to merge 2 commits into
Conversation
_findJumpTableSize takes the first `cmp reg, imm` in the 50-instruction
backtrack window as the table bound, without tying it to the jump index.
In 32-bit Rust float-formatting code an IEEE-754 classification sits a few
instructions ahead of a real 4-entry switch:
cmp ecx, 0x7f800000 ; f32 +infinity bit pattern
add edi, dword ptr [eax*4 + 0x493da0] ; the actual table
jmp edi
so the recovered bound became 0x7f800001 entries. Both table readers then
treated an unreadable entry as a reason to skip that index and keep going,
even though the read offset only increases: once a read falls outside the
mapped image every later one does too. The direct reader therefore spent
2.1 billion getBytes calls -- about 285 seconds -- on a single function,
which is more than the whole default analysis budget of 300 seconds. The
run was cut short right after the heuristic pass, so gap analysis, the
tailcall pass and the remaining candidates never ran, and the report came
back marked as a timeout with roughly half the functions of a full run.
Stop each scan at the first entry that cannot be read. This is output
equivalent -- the skipped iterations could never contribute a target -- and
the same holds for the negative-rebase guard, which is loop invariant and
so decides the whole scan on its first iteration. _resolveExplicitTable in
the same file already did this correctly.
On a 1.1 MB i686 PE that reproduces the report, analysis drops from 638 to
1.7 seconds and completes instead of timing out, recovering the identical
6209 functions the untimed run found.
Refs: danielplohmann#247
…ntry Both walks take their entry count from a 16-bit field in the image, so a truncated or corrupt table can declare up to 65535 entries. The dynamic method table and the ReSym method definition table each read one pointer per entry at a strictly increasing offset, and treated a read past the end of the mapped image as a reason to skip that entry rather than to stop -- so a bogus count was walked out in full, once per class candidate found. Stop both walks at the first unreadable entry. In the dynamic table the test is explicitly against None so that a genuine null slot in the middle of a table is still only skipped, matching the sibling walk's contract. This is the same defect the jump-table entry scans carried, at a lower amplitude: a 16-bit count instead of a 32-bit one.
|
Independent corroboration on a second, unrelated binary, since the repro attached to #247 was a single artefact and this confirms the diagnosis generalises.
So on this binary too: output-equivalent, and ~314× faster. The identical function count between the raised-budget unpatched run and the patched run is the same "identical set" property your validation table reports, reproduced independently. The Why it is easy to missThe truncated report does not look damaged. In my case it produced a plausible-but-wrong measurement that I filed as a negative result: I was measuring what fraction of each binary's functions are EH cleanup funclets, and this sample came out at 0.8% — the lowest of its entire class, which I recorded as "this one is not affected". Re-run with the budget raised it is 29.2%, comfortably in the affected group. The reason it lands that way is exactly the mechanism in your root-cause section: analysis stops after the call-graph pass, so the surviving functions are the call-reachable ones — which are precisely the non-funclets, since a cleanup funclet is reached only through the unwinder. The truncation therefore biases the result in a specific, plausible-looking direction rather than producing obvious garbage. Not a request for anything in this PR — the fix is the fix. Recording it because "a truncated report reads like a complete one" is the part a downstream consumer is most likely to get wrong, and I did. |
|
Thanks — the second sample is the useful half of this, and the funclet observation is worth more than the timing numbers. The bias direction is the part I had not stated explicitly: truncation keeps the call-reachable population, and an EH cleanup funclet is reached only through the unwinder, so it is systematically the first thing to disappear. Any per-binary ratio whose numerator is "functions not reached by a direct call" is therefore not merely noisy on a truncated report, it is skewed toward zero — which is exactly the shape of a result that gets filed as a negative rather than as a failure. That is what makes "a truncated report reads like a complete one" worth fixing on its own, so I have opened #253 for it: one warning at default log level when the analysis is cut short, emitted on the timeout callback every backend polls, plus an incompleteness marker in the report's string form. The |
Summary
Fixes the function-discovery shortfall reported in #247. Discovery is not blind on that binary — the analysis never finishes.
report.statuscomes back as"timeout", because a single function spends more than the entire default 300 s budget inside jump-table analysis, so gap analysis, the tailcall pass and every remaining candidate are skipped.Root cause
_findJumpTableSizetakes the firstcmp reg, immin the 50-instruction backtrack window as the table bound, without tying it to the jump index. In 32-bit Rust float-formatting code an IEEE-754 classification sits a few instructions ahead of a real switch:so the recovered bound is
0x7f800000 + 1= 2,139,095,041 entries. Both table readers then treated an unreadable entry as a reason to skip that index and keep going — but the read offset only increases, so once a read falls outside the mapped image every later one does too._extractDirectTableOffsetstherefore made 2.1 billiongetBytescalls, ~285 s, for one function;_resolveExplicitTablein the same file already stopped correctly.Because the stall is structural rather than clock-dependent, the truncated result is deterministic: the reporter's 1167 functions reproduce exactly on a much faster machine and on any timeout setting below the stall.
Profile of the single slow
analyzeFunctioncall:What changed
Each per-entry scan now stops at the first entry it cannot read, instead of running out the declared count:
JumpTableAnalyzer._extractDirectTableOffsetsand_extractRelativeTableOffsets(plus the negative-rebase guard in the latter, which is loop-invariant and so decides the whole scan on its first iteration).DelphiPythiaProvider._extract_dynamic_methodsandDelphiReSymProvider._parse_mdttake their entry count from a 16-bit field, so a corrupt table declares up to 65535 entries and each walk runs once per class candidate. In the dynamic table the new test is explicitly againstNone, so a genuine null slot mid-table is still only skipped.All five are output-equivalent: the skipped iterations could never contribute a target. No heuristic, threshold or report field changes.
The underlying misattribution — an unrelated
cmpbecoming a table bound — is deliberately left alone here. Tying the bound to the jump index changes which tables are recovered, so it belongs in its own change with a corpus measurement behind it; this PR only stops the bogus bound from being walked out.Validation
python -m pytest tests/ -q→ 1185 passed, 1 skipped, 1480 subtestsruff check .,ruff format --check .→ cleanmake typecheck→ exit 0, no new errorsdiff-cover coverage.xml --compare-branch=upstream/master --fail-under=100→ 6/6 changed lines coveredEnd to end on the 1.1 MB
i686-pc-windows-msvcPE from the issue:status: timeout, 1167 functionsstatus: ok, 6209 functions.textrecoveredRefs: #247