fix(intel): keep an early-return body in its function when it is analyzed first

[r0ny123]

Depends on #386 (builds on its jcc rule); the branch carries #386's commits until that merges.

What this changes

#386 keeps a jcc target that has nothing but a prologue behind it in the jumping function, but only when the jcc is decoded first. When the body's prologue outranks its entry in the candidate order (entry with four calls or fewer, or reached only through a pointer or a jump thunk), the body is already a function by then and the jcc took it as a tailcall. The jcc now undoes such a function and decodes its blocks in the entry, with the same _isEvidencedEntry rule. Gap functions are left alone, and the undone function's state is dropped from the tailcall pass so it is not reverted a second time.

How it was verified

Issue repro (entry called 0/1/4/5/8 times): one function in every case (before: split at 0, 1, 4). Added as a test, plus the pointer-reached-target test now also runs with the target analyzed first.

Against #386's head, per-function instruction sets compared (samples not bundled):

• official sqlite 3.46.1 DLL x86: 4 functions fewer. Each is the non-SSE fallback of a CRT float-to-integer helper (cmp [__isa_available], 2; jl fallback; ...; ret; fallback: push ebp), reached only by its entry's jcc; the entries are reached only through jump thunks. x64 identical.

• python.org 3.12.7 embeddable x86 and x64 (all DLLs/pyds incl. python312.dll, libcrypto, libssl, vcruntime) and the bundled x86/x64 fixtures: identical.

• A first version also undid gap functions and absorbed jump thunks, which lost 424 functions in libcrypto; that is why gap functions are excluded.

• make lint and make format

• make test-all (2282 passed, 2 skipped), ty clean

• CHANGELOG.md entry under ## [Unreleased]

• Golden fixtures unchanged

Closes #388

[danielplohmann]

Thanks. The #388 repro is fixed: one function for an entry called 0/1/4/5/8 times, where master splits at 0, 1 and 4. The #385 fixtures stay 11/11 everywhere, and the full suite passes. But the rule reaches much further than early-return bodies. Before this goes in I'd like it narrowed.

1. Recall loss on real x86 PEs (X86Backend.py, the branch for a body that was analyzed first)

I compared per-function instruction sets on 125 deduplicated PE32 files (tools, installers, CRT DLLs, mingw; not bundled), master against this PR. The PR finds 144,730 functions where master finds 144,766: 36 starts are absorbed into another function in 19 files, and none are gained. Only about 6 of them are the intended case, a body directly behind a ret and 5 to 0x4c bytes after its entry. The rest:

• Old CRT ftol2.asm: 24 starts in 12 files (Sysmon, Notepad++, pestudio, ProcessHacker, msvcp140d, …). _ftol2 is 16-aligned behind 8d 49 00 (lea ecx, [ecx]) padding. That padding isn't CC/90, so _isEvidencedEntry doesn't count it, and _ftol2_sse absorbs _ftol2. The _ftol2_pentium4 body is also absorbed, through a je, by the following _ftol2_sse_excpt, which is worse than master's over-split.
• Hotpatched functions: 4 starts. Each starts with mov edi, edi and is absorbed from 0x1547 to 0xcad0 bytes away, e.g. hollows_hunter32 0x494f50 by a floor-dispatch stub at 0x488480. mov edi, edi only ever opens a function.
• Delphi: 3 starts, 4-aligned behind 8d 40 00, absorbed by a function 0xa0 bytes after them.
• Other: 1 start, behind int3 but not 16-aligned, 0x796 bytes from its absorber.

Prototype guard: in the already-analyzed branch, only absorb when 0 < jump_destination - state.start_addr <= 0x100 and the byte before jump_destination is 0xC3. On the same 125 files that leaves exactly the 6 intended changes, with none of the 30 losses. Your tests still pass, and all three repros below come out like master. Counting 8b ff, the effective-NOP paddings (8d 40 00, 8d 49 00, 8d a4 24 …) and 4-byte alignment as entry evidence would also help, but adjacency is what separates an early-return body from a separate function.

2. An absorbed function is lost if the absorbing function aborts (RecursiveDisassembler.absorbFunction)

absorbFunction reverts B and marks it finished right away. If A then doesn't finalize, nothing undoes that, and B ends up in no function. A doesn't finalize after two suspicious all-zero instructions, when finalizeAnalysis returns False, or on a timeout partway through B's blocks. _revertGapFunction has _restoreRevertedGapFunctions for exactly this; the absorb path needs the same. _revertStoredFunction also leaves B's code, data and call refs registered, so they dangle after an abort.

3. A gap function absorbing a distant B skips the gap in between

Before this PR, all non-gap candidates were analyzed before the gap pass, so a gap function never absorbed one. Now it can. getNextGap then resumes at function_borders[A][1], which is past B, so gap candidates between A and B (e.g. jump thunks) are never tried. The adjacency guard from 1 covers this as well.

Repros: 32-bit buffer at 0x401000, 0x200 bytes of CC. All three re-run on 0f7bed3:

from struct import pack
BASE = 0x401000
rel = lambda src, size, dst: pack("<i", dst - (src + size))
BODY = b"\x55\x8b\xec\x8b\x45\x08\x5d\xc3"
A, B, C, E = BASE + 0x80, BASE + 0xA3, BASE + 0x180, BASE + 0xC0
jcc = lambda a, b, tail=b"\x31\xc0\xc3": (a, b"\x85\xc9\x0f\x85" + rel(a + 2, 6, b) + tail)
ptr = (C, b"\x55\x8b\xec\x68" + pack("<I", B) + b"\xff\x10\x83\xc4\x04\x5d\xc3")
# calls(BASE, [A, C]) places one `call` to each at BASE, then `ret`
# 1: [calls(BASE,[A,C]), jcc(A,B), (B,BODY), ptr]                          master B=FN    | PR B inside A
# 2: [calls(BASE,[A,C]), jcc(A,B,b"\x00\x00\x00\x00\xc3"), (B,BODY), ptr]  master B=FN    | PR B lost
# 3: B2=BASE+0x103: [jcc(A,B2), (E, b"\xe9"+rel(E,5,B2)), (B2,BODY)]       master A,E,B=FN | PR E lost, B inside A

Smaller:

• Which function reaches B first decides the result: with two jcc sources, the first wins and the other branches into the middle of A. The guard narrows this a lot. I'm only noting it.
• CHANGELOG: say whether the sqlite figure was reproduced, add the measured tree's sha, and state the cost. As measured above, the cost isn't zero yet.
• A subtest with an unaligned or Delphi-padded target in X86PointerReachedFunctionBehindPaddingTest would have caught 1.

Not in the 4.10.0 milestone for now. Once the guard is in and the PE sample is clean, it can go into the next release.

[r0ny123]

Thanks, all three addressed in dd41bf3 (CHANGELOG in ac05204).

1. Your guard is in: a function analyzed first is only taken back when it starts right after a ret (C3) and 0 < target - entry <= 0x100. The absorb branch is otherwise unchanged (non-gap, no entry evidence).
2. An absorbed function is analyzed again when the absorbing one doesn't finish (suspicious-instruction abort, finalizeAnalysis failing, or the table-retry pass not reaching it). Its code and data refs are now removed when it is undone, so nothing dangles.
3. Covered by the adjacency guard.

Tests: your three layouts are in X86FunctionAnalyzedBeforeItsJumperTest, plus targets behind lea ecx, [ecx] (16-aligned), lea eax, [eax] (4-aligned), unaligned int3, a mov edi, edi entry, and a target before the jumper, and a body whose absorbing function aborts. All fail on 0f7bed3 except the last subtest; all pass on master except the #388 repro itself.

Measured dd41bf3 against 0802f7c on 255 x86/x64 PEs (python.org 3.5.4/3.6.8/3.8.10/3.12.7 embeddable, VS2008-built cp27 wheels, mingw-w64 DLLs, sqlite 3.46.1, bundled fixtures): 12 functions fewer in 3 files, all __isa_available-gated float-to-integer helper fallbacks directly behind the ret, nothing else moves. None of my files has the old _ftol2 padding, hotpatch entries or Delphi, so the 125-file run is still the one that matters there; those shapes are covered by the subtests. Full suite and ty pass; runtime unchanged on sqlite x86.

On the first-source-wins point: agreed, left as is.

[danielplohmann]

Re-review of 043fb89 against af37541 (v4.10.0).

The guard and the restore path work as described. On real binaries the result is clean now. Two holes remain in the code. The first is the remaining half of last round's point 3, and it is the reason for the verdict. The second is small.

What holds

• The three layouts from the last round now give the same functions as master: 1 keeps B as a function, 2 keeps B as a function, 3 keeps A, E and B. The #388 layout is fixed: A now has 9 instructions, where master split it into 4 + 5. This holds with one call to A and with none.
• X86BodyBehindAnEarlyReturnTest::testTheBodyStaysInTheFunctionWhenItIsAnalyzedFirst fails 3/3 subtests on master and passes on the PR. The new X86FunctionAnalyzedBeforeItsJumperTest cases pass on both trees, which is expected because they pin master behaviour. If I stub out _restoreAbsorbedFunctions, testABodyIsAnalyzedAgainWhenTheFunctionTakingItAborts fails, so that test does guard the restore. The related intel tests (testIntel*, testTailcallFlagIndependence, testGapScanTimeout, testCandidateSafeguards) pass: 147 passed.
• Refs: I put a call into B and compared code_refs_from/code_refs_to. For absorb, and for absorb followed by an abort of A, both are identical to master. from and to stay consistent, with no dangling refs and no duplicates.
• The table-retry path and the suspicious-instruction abort both restore B correctly. In the retry, B is no longer a function and gets decoded as a plain block. If the retry does not reach B, the outer restore picks it up.

Corpus. A benign PE corpus (an MSVC VS6..VS2019 debug/release build matrix, Windows system files, other benign third-party PEs) plus the floss go/rust/src test binaries, not bundled: 494 PEs (439 x86, 55 x64, ≤10 MB, deduplicated by sha256). Of these, 290 x86 files contain 8d 49 00 padding, 148 contain 8b ff 55 8b ec, and 5 carry Delphi markers.

• 70 function starts fewer in 55 files (967,023 → 966,953). None gained. No instruction changes owner other than these.
• Every one of the 70 is absorbed by the function directly in front of it, 3..0x4c bytes after its entry. In every case the code from the entry decodes contiguously up to the body and ends in ret:
  • 54 are _RTC_CheckEsp in the debug builds (jne esperror; ret; esperror: push ebp ...), VS2003..VS2019.
  • 12 are the __isa_available-gated float-to-int helpers (cmp; jl body; ... ret). This is the same shape Rony reported.
  • 4 are LLVM shrink-wrapped functions in a Rust 1.59 i386 binary (test dl, dl; je body; mov al, 1; ret; body: push ebp ...).
• 0 starts lost from the system files, the third-party PEs or the release builds. 0 starts absorbed across another function. 0 lost entirely.
• Runtime is 422.8 s on master and 422.1 s on the PR.

I can't tell if this corpus overlaps the 125 files from last round, which are gone. It does contain the lea ecx,[ecx] and hotpatch shapes, and none of them moved.

1. A gap function can still absorb across another function and swallow the gaps in between. _isBodyBehindReturn checks that a ret sits in front of B. It does not check that this ret belongs to A. With a gap function A and an unreferenced function between A's ret and B, A's border grows to B's end, and getNextGap(dont_skip=True) resumes behind that border. The function in between is never tried. This is last round's point 3 again, now inside 0x100 bytes. Same buffer as last round (BASE = 0x401000, A = BASE + 0x80, 0x200 bytes of CC):

   G = b"\x31\xc0" + b"\x40" * 13 + b"\xc3"                # xor eax, eax; inc eax x13; ret
   [jcc(A, A + 0x30), (A + 0x20, G), (A + 0x30, BODY)]       master A, A+0x20 (15 ins), A+0x30 | PR A only (9 ins)
   [jcc(A, A + 0x30), (A + 0x10, b"\x31\xc0\x40\xc3"),
    (A + 0x27, b"\x8b\xff\x55\x8b\xec\x33\xc0\x5d\xc3"), (A + 0x30, BODY)]
                                                             master A, A+0x10, A+0x27, A+0x30 | PR A, A+0x27
   

   In the first layout, a 15-instruction function is lost outright. When A is called, so not a gap function, nothing is lost, but B still joins A across A+0x20, as a non-contiguous body.

   Proposed fix: also require that the bytes from the jcc's fall-through up to B decode contiguously, end exactly at B, and contain no int3/nop and no start of an existing function. That is a ret that ends A's own straight-line code.

   I prototyped this as a ten-line _fallsThroughToBody(d, i_address + i_size, jump_destination) next to _isBodyBehindReturn. Results:

   • All four layouts above match master again.
   • Layouts 1–3 and #388 are unchanged.
   • testShrinkWrappedEntry.py passes 28/28.
   • The 55 changed corpus files give results identical to the PR, so all 70 intended absorptions are kept, including the 12 __isa_available helpers, whose SSE path has two extra rets.

   Please add the first layout as a test.

2. A timeout between the absorb and B's block leaves B in no function. The restore only runs when start_addr not in self.disassembly.functions. When the poll trips at the block right after the absorb, the loop breaks and A finalizes with only its own blocks. Repro: the #388 layout, with _TIMEOUT_POLL_BLOCKS = 1 and a timeout that trips once absorbFunction returned True. Master gives A 4 + B 5. The PR gives A 2 and B nowhere. With 256 the window is the one poll right after the absorb, because the queue is LIFO and B is decoded next, so this is rare. The fix is cheap: call _restoreAbsorbedFunctions(state.absorbed_functions) unconditionally after finalizeAnalysis. It already skips everything that is in ins2fn.

3. CHANGELOG. Prefix, subsection, Compatibility bullet and the measured tree are fine. dd41bf3 has the same src/ as the head, apart from __version__.

   • Close with the PR number, (#388, #389), not just the issue. Every other entry links its PR.
   • At 10 lines, the Fixed bullet is past the ~6-line budget. The corpus list can move into the PR body.
   • "Not reproduced by a reviewer" can now read: Reproduced by reviewer on 494 benign PEs (not bundled): 70 starts fewer in 55 files, all bodies behind their entry's ret (_RTC_CheckEsp, __isa_available helpers, LLVM shrink-wrapped functions), runtime unchanged. Update it if 1. changes the figures. The prototype did not change them.

The restore-on-abort work is good, and the corpus no longer shows any loss. Point 1 is the same mechanism as last round's point 3, and it can lose a whole function. I'd like it closed before merge. Point 2 is a one-line change.

Verdict: REQUEST_CHANGES

[danielplohmann]

Thanks, both points from the last round are fixed on c999339. All ten repro layouts, including the gap function absorbing across A+0x20, now give master's functions, except the two #388 layouts this PR is for. A one-shot timeout right after an absorb gets the body back as its own function. One thing I'm leaving as is: if the timeout stays tripped, the body is still lost, while master keeps it. Nothing is analyzed after that point anyway, so it is not worth more code. I merged master in after #394/#395/#396 (b33ff8b, CHANGELOG only) and re-ran the tests and repros on top of AddressRefs: unchanged.