Skip to content

PE: Back the whole image the section table declares - #790

Open
zardus wants to merge 2 commits into
masterfrom
feature/cle-pe-reloc
Open

zardus wants to merge 2 commits into
masterfrom
feature/cle-pe-reloc

Conversation

@zardus

@zardus zardus commented Aug 26, 2026 •

Copy link
Copy Markdown
Member

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Problem

cle backs less of a PE than the object says it covers. On binaries/tests/i386/windows/rain32.upx, whose .rsrc declares memsize=0x1000 over filesize=0x600:

<PE Object rain32.upx, maps [0x400000:0x40afff]>
declares                      : 0x400000 .. 0x40afff  (0xb000 bytes)
mapped image the backend built: 0xa600 bytes

ld.memory.load(0x40afff, 1)  -> KeyError: 4239359    # last address the object declares
ld.memory.load(0x40a600, 1)  -> KeyError: 4236800    # the .rsrc tail

Every PE loses the last byte of its image this way, and a section whose virtual size exceeds its raw data loses that whole tail: 106 of the 106 PEs angr/binaries tracks at fc07821c back less than they declare. A base relocation whose field lands in the gap then gets a short buffer and IMAGE_REL_BASED_HIGHLOW raises struct.error out of Loader, losing the object entirely.

Root cause

Two independent off-by-a-region errors in cle/backends/pe/pe.py.

Backend.max_addr is the last address an object covers, not one past it, but the clamp read if self.max_addr - self.min_addr < len(mapped_image) and truncated to mapped_image[: self.max_addr - self.min_addr], one byte short of the declared range.

_get_memory_mapped_image() separately stops at the last section's raw data. A section is mapped over its whole Misc_VirtualSize, but the zero padding that backs that is emitted ahead of the next section, so the last section of all gets none. .rsrc is last in rain32.upx, which is why the 0xa00 bytes it declares beyond its raw data are in no object at all.

Fix

The clamp spans max_addr - min_addr + 1, and the image is zero-padded up to the highest virtual address + Misc_VirtualSize of any section whose raw data the file holds in full. Same binary, same commands:

declares                      : 0x400000 .. 0x40afff  (0xb000 bytes)
mapped image the backend built: 0xb000 bytes

ld.memory.load(0x40afff, 1)  -> 00
ld.memory.load(0x40a600, 1)  -> 00

Deliberately not done: a section whose raw data lies outside the file keeps its unbacked tail, because those bytes are unknown rather than zero. test_loading_incomplete_pe_file pins that behaviour, and widening the fix to cover it fails that test.

Testing

tests/test_pe.py::test_mapped_image_covers_max_addr requires ld.memory.load(min_addr, max_addr - min_addr + 1) on binaries/tests/i386/simple_windows.exe to return the full declared length; test_mapped_image_covers_uninitialized_tail requires rain32.upx's .rsrc tail to read as zeroes. Both fail on the commit before them and tests/test_pe.py is 17 passed here. Across those same 106 PEs, 1 still backs less than it declares, and that is the out-of-file-section case above; CFGFast(normalize=True) over the public PE fixtures the change touches returns identical function and node address sets on both revisions.

One consumer, flagged rather than changed: zero-filling a section's unmapped tail removes the None return from CFGFast._load_a_byte_as_int for those addresses, which the open angr pull request 6943 depends on in its is_sz = False branch, so the two decide the recovered string set between them and merge order matters. Detail in the coupling note on this PR. cle#835 edits the same PE code and its description says this change's image_end becomes too small once it lands; measured, the two are independent and neither loses a byte, which the second coupling note records.

Validation: #790 (comment)

session: sharpen

@zardus

zardus commented Aug 26, 2026 •

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Validation record for head 0015420b2920a8faebb8920967882071787f4c0f against baseline 0e77ade3c39a3cee05f65051e57955675e1ac21b.

  • Regression: python -m pytest tests/test_pe.py -k mapped_image_covers — each test fails on the commit before it: test_mapped_image_covers_max_addr at the baseline (the load returns one byte fewer than the object declares), test_mapped_image_covers_uninitialized_tail at b43adc6 (.rsrc in tests/i386/windows/rain32.upx declares 0x1000 bytes over 0x600 of raw data and the last 0xa00 are unbacked)
  • Focused: python -m pytest tests/test_pe.py — 16 passed
  • Full suite: python -m pytest tests/ — 241 passed, 9 skipped; the baseline collects 240 passed and 9 skipped, so the difference is the one added test
  • Lint/type: pylint and pyright, scored per changed file against the merge base — pylint 10.00 unchanged on both; pyright badness 0.6420 -> 0.6352 for cle/backends/pe/pe.py and 0.1278 -> 0.1194 for tests/test_pe.py
  • Mapping extent, every PE in angr/binaries loaded through cle.Loader: 92 of 92 back less than [min_addr, max_addr] at the baseline, 4 after the first commit, 1 after the second. The remaining one is a section whose raw data lies outside the file, left alone deliberately
  • Paired load census, both commits as the only variable, over 92 PEs in angr/binaries, 24 in DecBench and 377 from a local collection that cannot be published — 45 of the fixtures are byte-identical to members of that collection, so the union is 422 distinct binaries. 493 candidates in and 493 records out on each arm, every record ok
  • Regressions first: 0 images got shorter, 0 objects moved min_addr or max_addr, 0 objects gained a shortfall, and the 480 records whose image length is unchanged hash byte-identical between arms. 12 shortfalls close and 3 records remain, all of them the out-of-file-section case above
  • Cost: 72,615 bytes of new zero backing over 13 of 493 loads and none over the other 480; the largest single addition is 38,511 bytes and the largest in angr/binaries is 3,584, in tests/x86/windows/packed_pe32.exe
  • No analysis sees the difference: CFGFast(normalize=True) over the public PE fixtures the change touches — rain32.upx, packed_pe32.exe, 9f2ef84bde1e..., a94bbeed0ef5... and simple_windows.exe — returns identical function and node address sets on both arms, 0 gained and 0 lost on each. The bytes now backed are bytes nothing could reach
  • Downstream: the angr tests that load these fixtures, tests/analyses/test_unpacking.py and tests/analyses/decompiler/test_decompiler.py::TestDecompiler::test_decompiling_no_phivar_in_call_statements, with cle as the only variable between arms — 8 passed on each. PackingDetector still reports rain32.upx packed and still raises nothing on a94bbeed0ef5...
  • Workspace gate: NOT re-run on this head. A corpus sweep holds the workspace, and entering its shell reinstalls editables under the running jobs, so the scoped equivalent ran instead — cle's own suite, the merge-base lint and type comparison, and pre-commit run --all-files (every hook passed). The gate figures in the earlier revision of this comment describe b43adc6, not this head; angr, claripy, pyvex, archinfo, pypcode and angr-management were exercised only as wheels and this run proves nothing about them

Caveats: one PE in angr/binaries still backs less than it declares, because a section whose raw data lies outside the file has unknown contents rather than zero ones; test_loading_incomplete_pe_file pins that behaviour and widening the fix to cover it fails that test, so it wants its own change and its own evidence. angr/binaries holds no .NET assembly and no PE with a base relocation in the final bytes of its image, so no committed fixture reproduces the loader failure end to end; the regressions assert the mapping shortfall instead, which is the defect itself. The objects that fail to load on the baseline are third-party samples that cannot be committed anywhere.

Re-keyed 2026-08-29. Re-keyed from 6ae432998126bf764e9be007eadc451f0ecd1fcb on baseline 46a37333f4f59b0facf8774ee743ebc4cc074e9b after a rebase onto eac0e5540516b9199dd6a91933e80dc774ea3eac. git range-diff marks the first commit ! rather than =, but the branch's own payload is unchanged: comparing each head's git diff against its own merge base, no + or - line differs, and the whole difference is index blob hashes and @@ offsets.

What did move is the baseline, and it moved through this change's own file. Master gained four commits between the two, one of them eac0e554 "PE: read the loading environment from the optional header (#800)", which adds 28 lines to cle/backends/pe/pe.py and 17 to tests/test_pe.py. So the first bullet above — each regression failing on the commit before it — is a baseline-relative result measured against 46a37333, and it has not been re-run against eac0e554. The suite, focused, lint/type, census and downstream figures are properties of the head rather than of the baseline and carry unchanged.

Hosted CI at 9eab3c507a539bbd785e9661b919185fe0b0fb20 is green: 18 of 18 check runs concluded success (https://github.com/angr/cle/actions/runs/33270384190) and both legacy commit statuses are success.

The corpus measurement posted separately on this pull request (#790 (comment)) — the five objects that raise struct.error at cle/backends/pe/relocation/generic.py:67 on master and load on this branch — had been written with bold before-and-after headings, which made a population measurement read as a second reproducer-output comment. Those headings are now prose; no figure in it changed. The one output comment on this pull request remains #790 (comment), the loader output for tests/i386/windows/rain32.upx, captured on cle master d2ecea0 against branch head 6ae4329; the branch's payload is byte-identical at the current head, so that output still describes it.

Re-keyed 2026-09-08. Re-keyed from 9eab3c507a539bbd785e9661b919185fe0b0fb20 on baseline eac0e5540516b9199dd6a91933e80dc774ea3eac after a rebase onto cle master 0e77ade3c39a3cee05f65051e57955675e1ac21b. The branch had gone stale: git merge-base --is-ancestor origin/master 9eab3c50 exits 1, master was 7 commits ahead of the old merge base, and the 20 green checks above were produced on 2026-08-29.

The change did not move. git range-diff eac0e5540516b9199dd6a91933e80dc774ea3eac..9eab3c507a539bbd785e9661b919185fe0b0fb20 0e77ade3c39a3cee05f65051e57955675e1ac21b..0015420b2920a8faebb8920967882071787f4c0f marks both commits =, and tests/test_pe.py has the same blob at both heads. cle/backends/pe/pe.py does not: master's b6ff025b ("Golang: Read the Go pclntab on Mach-O and PE, not only ELF", #808) added four lines to that file — a module-level import, and a comment, an assignment and a blank line in PE.__init__, in the function this change edits. So the second and third of the four conditions that let a rebase skip re-review — an identical blob for every touched file, and a base that did not touch those files — fail here.

The two commit messages are left as they were written, so that the rebase stays a replay anybody can check with range-diff. One of them says "88 of the 92 by exactly one byte", which was the figure when it was written, against angr/binaries 8646be4e; the description and this record carry the current one.

Everything below is re-measured at 0015420b2920a8faebb8920967882071787f4c0f on baseline 0e77ade3c39a3cee05f65051e57955675e1ac21b, with angr/binaries at fc07821c.

  • Regression, both arms, tests read from the branch worktree and the package taken from a build of each revision: pytest --import-mode=append -q tests/test_pe.py -k mapped_image_covers is 2 failed against a build of master 0e77ade3 (test_mapped_image_covers_max_addr on the assertion, test_mapped_image_covers_uninitialized_tail on KeyError: 4236800) and 2 passed at this head. That is the baseline-relative result the 2026-08-29 re-key recorded as not re-run; it has now been run against the current base
  • Focused: pytest tests/test_pe.py — 17 passed
  • Full suite: pytest tests/ — 263 passed, 9 skipped at this head against 261 passed, 9 skipped on master 0e77ade3, so the difference is still exactly the two added tests
  • Lint/type: run-ci-diff-checks.py against the new base, exit 0 — pylint 10.00 -> 10.00 on both changed files, pyright errors 52 -> 52 for cle/backends/pe/pe.py and 4 -> 4 for tests/test_pe.py. pre-commit run --files cle/backends/pe/pe.py tests/test_pe.py — exit 0, every hook passed or skipped
  • Mapping extent, every PE angr/binaries tracks, loaded through cle.Loader(path, auto_load_libs=False): 106 of 106 back less than [min_addr, max_addr] on master, 1 of 106 at this head. The corpus was 92 files when this record was first written; it is 106 now, which is why the figure moved. The residual is tests/i386/windows/a94bbeed0ef51db3d3964bb0cc2cbed0adab0e47997d88f34daa92faa1a91e8a, short by 10377 bytes — the out-of-file-section case, which is cle#794. 0 load errors on either arm
  • New control for the base change, since #808 landed in the function this change edits: PE.gopclntab and the symbol count for all 106 PE files are byte-identical between a master build and this head. 11 of the 106 carry a Go pclntab and all 11 register the same way on both arms, so widening the mapped image does not change what register_gopclntab_symbols finds
  • Interaction with cle#835, which edits the same PE code: measured separately in the coupling note on this pull request. tests/test_pe.py on a tree carrying both is 19 passed, and the mapping-extent census on that tree is the same 1 of 106

Carried forward from the measurements above, which are properties of an unchanged payload rather than of the baseline: the paired 493-object load census, the zero-backing cost figures, the CFGFast(normalize=True) identical-output result, and the downstream angr tests.

Hosted CI at 9eab3c507a539bbd785e9661b919185fe0b0fb20 was 20 of 20 green, and that result belongs to the old base. The checks at this head are what count; the complete workspace test gate was not run on this head — the scoped evidence above, cle's own suite on both sides plus the merge-base lint and type comparison, is what this record rests on, and the hosted matrix is what tests the rest of the ecosystem.

@angr-bot

Copy link
Copy Markdown
Member

Corpus decompilation diffs can be found at angr/dec-snapshots@master...angr/cle_790

@zardus zardus changed the title PE: Back the last byte of the image the section table declares PE: Back the whole image the section table declares Aug 26, 2026
@zardus

zardus commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Coupling note, which the description above does not state: this change has a consumer in angr.

Zero-filling a PE section's unmapped tail removes the None return from CFGFast._load_a_byte_as_int for those addresses. CFGFast._scan_for_printable_strings distinguishes exactly that case from a real zero byte — if val is None: break versus if val == 0: break — and angr/angr#6943 adds is_sz = False to the None branch so such a run needs 13 printable bytes instead of 3. With this PR merged, PE runs take the val == 0 branch instead and qualify at 3.

Verified on binaries/tests/i386/windows/rain32.upx: .rsrc memsize=0x1000 against filesize=0x600, and ld.memory.load(0x40a600, 1) raises KeyError on current cle.

So the two PRs decide the recovered string set between them and the result depends on merge order. Flagging it here rather than changing either.

@zardus

zardus commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Full loader output for tests/i386/windows/rain32.upx in angr/binaries, before and after this change. The reproducer is cle.Loader(path, auto_load_libs=False) followed by the two ld.memory.load calls shown; .rsrc is the last section and declares memsize=0x1000 over filesize=0x600.

Before — the backend builds 0xa600 bytes for an object that declares 0xb000, so the last address of the image and the whole uninitialised .rsrc tail are in no object at all:

cle master (d2ecea0)
binary                       : tests/i386/windows/rain32.upx
cle                          : master d2ecea0
object                       : <PE Object rain32.upx, maps [0x400000:0x40afff]>
declares                     : 0x400000 .. 0x40afff  (0xb000 bytes)
mapped image the backend built: 0xa600 bytes

section table:
  UPX0   vaddr=0x00401000 memsize=0x07000 filesize=0x00000
  UPX1   vaddr=0x00408000 memsize=0x02000 filesize=0x01800
  .rsrc  vaddr=0x0040a000 memsize=0x01000 filesize=0x00600

ld.memory.load(0x40afff, 1)  -> KeyError: 4239359    # last address the object declares
ld.memory.load(0x40a600, 1)  -> KeyError: 4236800    # .rsrc tail: memsize 0x1000 over filesize 0x600

After — the image spans the range the object declares, and the tail reads as the zeroes a PE section's virtual size means:

with this change (6ae4329)
binary                       : tests/i386/windows/rain32.upx
cle                          : this change 6ae4329
object                       : <PE Object rain32.upx, maps [0x400000:0x40afff]>
declares                     : 0x400000 .. 0x40afff  (0xb000 bytes)
mapped image the backend built: 0xb000 bytes

section table:
  UPX0   vaddr=0x00401000 memsize=0x07000 filesize=0x00000
  UPX1   vaddr=0x00408000 memsize=0x02000 filesize=0x01800
  .rsrc  vaddr=0x0040a000 memsize=0x01000 filesize=0x00600

ld.memory.load(0x40afff, 1)  -> 00    # last address the object declares
ld.memory.load(0x40a600, 1)  -> 00    # .rsrc tail: memsize 0x1000 over filesize 0x600

@zardus
zardus force-pushed the feature/cle-pe-reloc branch from 6ae4329 to d34beb5 Compare August 29, 2026 06:47
@zardus

zardus commented Aug 29, 2026

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Rebased onto cle master eac0e554; the branch was CONFLICTING after #800 merged. New head 9eab3c50.

The conflict was tests/test_pe.py, where #800 added test_uefi_image_is_not_windows at the same point this branch adds test_mapped_image_covers_max_addr. Both are kept; the pe.py hunks merged untouched.

The change itself did not move: comparing the branch's own diff before and after the rebase with context discarded gives a byte-identical set of added and removed lines.

Validation at the new head: pytest tests/test_pe.py — 17 passed. The merge-base lint and type comparison that ci / Lint and ci / Typecheck apply to changed files reports no regression at the new base.

@zardus
zardus force-pushed the feature/cle-pe-reloc branch from d34beb5 to 9eab3c5 Compare August 29, 2026 19:15
@zardus

zardus commented Aug 29, 2026 •

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

A separate corpus measurement, from a sweep triaging its own unowned failure
classes rather than from this branch's own validation, so the two denominators
do not get confused.

This change does not only widen the mapping. It clears a hard load failure:
struct.error: unpack requires a buffer of 4 bytes raised from
cle/backends/pe/relocation/generic.py:67 in IMAGE_REL_BASED_HIGHLOW.value.
The sweep had it filed as two separate open defect classes, and neither had a
candidate, because the index looks for a fix touching the frame that raises and
the fix is in cle/backends/pe/pe.py.

Mechanism. The clamp in PE.__init__ compares against and slices to
self.max_addr - self.min_addr, but Region.max_addr is the last covered
address rather than one past it, so the backer ends one byte short of the
declared image. Clemory.load returns up to n bytes and stops at the first
unallocated region instead of raising, so a base relocation whose 4-byte field
happens to end on that last address gets a 3-byte buffer and struct.unpack
raises. On one of the affected objects the relocation is at RVA 0xa200c, the
declared image ends at 0xa2010, and the byte at 0xa200f was present in the
mapped image and thrown away.

Method. Each arm is a git worktree of cle at one revision, prepended to
sys.path with any editable meta-path finder removed, asserting in-process that
cle.__file__ resolves inside that arm. Same objects by sha256 in every arm,
cle.Loader(path, auto_load_libs=False), one fork per object.

On cle master c7e0d4db, 5 of 5 affected objects raise at
generic.py:67; on this branch at 9eab3c50, 5 of 5 load. Over the
whole 385-object collection those 5 are the only regression-relevant change —
375 loaded on master and none of them changes class here.

Six pull requests the index offered as candidates on the two rows, plus the
current master head, each clear 0 of the 5, measured the same way: they are in
the ELF, COFF and DWARF paths, or in the PEReloc base that this relocation
class overrides.

Rate and denominator. 5 of 385 in that collection is 1.30% (Wilson 95%
0.56–3.00). The crash is much rarer than the under-backing your record above
already quantifies, because it needs a base relocation landing on exactly the
lost byte: 0 of 3,422 PE objects in a separate 11,989-object uniform random
draw hit it, so on that draw the rate is under 0.12% at 95%. All five affected
objects are 32-bit x86 PE, 620–660 KB, one symbol and three relocations each —
one DllImport, one IMAGE_REL_BASED_ABSOLUTE, and the single
IMAGE_REL_BASED_HIGHLOW that fails.

The corpus is not redistributable, so the objects are described by format,
architecture and size rather than named; none of the five is byte-identical to
anything tracked in the binaries repository, and none of the 95 PE fixtures
there carries a relocation on the lost byte, which is why no public reproducer
for this crash exists.

session: sharpen

zardus and others added 2 commits September 8, 2026 17:44
Backend.max_addr is the last address an object covers, not one past it, so the
mapped image spans max_addr - min_addr + 1 bytes. The backend clamped it to the
difference alone and dropped the final byte of the last section. The clamp is
not a rare path: _get_memory_mapped_image() builds from raw section data and
pads between sections, so it usually runs past the last section's virtual end
and the branch is taken. Every PE in angr/binaries ends up with part of its
declared range unbacked, 88 of the 92 by exactly one byte.

Clemory.load returns up to the length it is asked for, so a base relocation
whose field ends on the missing byte reads short and IMAGE_REL_BASED_HIGHLOW
raises struct.error out of Backend.relocate, through Loader, and out of
Project: the binary does not load at all. Five PE objects in a corpus sweep
failed this way, each one a .NET assembly whose single fixup patches the
operand of the jmp [_CorExeMain] stub in a sixteen-byte final section.

Loader already sizes an object as max_addr - min_addr + 1 and hands the next
one the space above max_addr, so the byte was already this object's own.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A section is mapped over its whole virtual size, and the bytes past the raw
data the file holds for it are zero. That padding is emitted ahead of the
next section, so the last section of all has nothing to back its own, and
the image ends at its raw data while max_addr covers its virtual size.

A section the file cuts short is excluded: those bytes are unknown rather
than zero, which is what PE.Preserve incomplete sections already decided.
@zardus
zardus force-pushed the feature/cle-pe-reloc branch from 9eab3c5 to 0015420 Compare September 8, 2026 19:04
@zardus

zardus commented Sep 8, 2026

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Second coupling note. cle#835 changes PESection.memsize to the larger of Misc_VirtualSize and
the section's raw size, bounded by the file and by SizeOfImage. Its description says that makes
this change's image_end "too small". That is true of image_end, and it costs the image no
byte. The measurement is below; it is what decided not to change this pull request.

image_end here is a max over va_adj + Misc_VirtualSize, so once #835 raises memsize past
Misc_VirtualSize on a section, image_end sits below the top of the image. On a tree carrying
both changes that is 98 of the 106 PE files angr/binaries tracks at fc07821c; on this
pull request's own head, where memsize is still Misc_VirtualSize, the same count is 1 of 106.
But image_end governs only the zero padding that backs a section's virtual size beyond its
raw data, and raising it to cover the larger memsize turns out to change nothing about the
image that comes out. That is measured below rather than argued.

cle.Loader(path, auto_load_libs=False) over the same 106 files, counting objects that back
fewer bytes than [min_addr, max_addr] declares:

cle master 0e77ade3                    106 of 106 short   (102 of them by exactly 1 byte)
master + this PR                         1 of 106 short   (the cle#794 case, short by 10377)
master + this PR + cle#835               1 of 106 short   (same file, same amount)

0 load errors on every arm. The variant formula — image_end = max(image_end, va_adj + max(Misc_VirtualSize, size)), with size the file-bounded raw size the loop already computes —
was measured against this one: it changes the length of the mapped image on
0 of the 106. This pull request keeps Misc_VirtualSize.

tests/test_pe.py on the combined tree is 19 passed: this change's two regressions and #835's two
both hold with the other applied. The two are independent and can land in either order.

session: sharpen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants