Skip to content

Make loader memory reads side-effect free - #788

Closed
zardus wants to merge 2 commits into
masterfrom
feature/clemory-side-effect-free-reads
Closed

zardus wants to merge 2 commits into
masterfrom
feature/clemory-side-effect-free-reads

Conversation

@zardus

@zardus zardus commented Aug 25, 2026 •

Copy link
Copy Markdown
Member

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Problem

next(memory.backers())[1] exposes mutable loader storage. Mutating that object can change later reads.

Root cause

Public and trusted internal readers shared the same iterator.

Fix

Return detached snapshots from public backers() while retaining a protected zero-copy iterator for trusted reads. Cache snapshots by semantic and layout identity, and propagate valid nested-memory changes to owners exactly once.

Testing

tests/test_clemory.py covers detached aliases, address 0x1000 nested reads, encrypted ranges, overlap and cycle rejection, copies, pickle restoration, and legacy state. Validation: #788 (comment)

session: sharpen

@angr-bot

Copy link
Copy Markdown
Member

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

@zardus

zardus commented Aug 27, 2026 •

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Validation record for head d341e92824610e4400225ec697b97848c4fb64cb against baseline a4fb8003198229d33c84df6a82f749729232fd31.

Rebase refresh:

  • Previous PR head: ff3cce77acacacf74eb0ad854d66a8f14def2ddd.
  • Previous merge-base: d2ecea068794d20b1f14d90eecc1bc4bc4cfa431.
  • Current base: master at a4fb8003198229d33c84df6a82f749729232fd31.
  • Current PR head read back from GitHub: d341e92824610e4400225ec697b97848c4fb64cb.
  • Rebase was clean. Range-diff: 1: 41c4649 = 1: 60de0a3 Make loader memory reads side-effect free; 2: ff3cce7 = 2: d341e92 Cache detached Clemory byte snapshots.
  • Changed-file list and raw changed blob OIDs are identical before and after the rebase.

Hosted validation:

  • Whole pull_request run for exact head d341e92824610e4400225ec697b97848c4fb64cb: https://github.com/angr/cle/actions/runs/33161822296
  • Result: terminal success, updated 2026-08-28T10:37:02Z.
  • All 18 named workflow jobs/check runs completed with success, including Build, Lint, Typecheck, Test (0)..(9), Decompiler Snapshot Testing (0), Publish Unit Tests Results, Pyodide, Windows, and macOS.
  • GitHub's PR status rollup also returned two nameless StatusContext objects with null status/conclusion/detailsUrl; no named check was failing or pending.

Producer/consumer audit:

  • No coupled consumer PR is required for this PR.
  • Current angr and angr-management backers() consumers are read-only and compatible with detached public snapshots.

Review/status inventory:

  • PR is open, mergeable, and mergeStateStatus is CLEAN.
  • No reviews were present in the final inventory.

Local validation:

  • No local tests, Nix, native builds, imports, or source/ref changes were run in this refresh by instruction. This record relies on clean rebase identity checks plus the exact-head hosted whole CI run above.

session: sharpen

@zardus
zardus force-pushed the feature/clemory-side-effect-free-reads branch from 2eec323 to ff3cce7 Compare August 28, 2026 01:07
@zardus
zardus force-pushed the feature/clemory-side-effect-free-reads branch from ff3cce7 to d341e92 Compare August 28, 2026 10:02
@zardus

zardus commented Aug 28, 2026 •

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Full public-backer mutation result for the Clemory regression, before and after this change.

Before — the object returned by backers() aliases loader storage:

CLE baseline
backer = next(memory.backers())[1]
backer[0] = 0x41
memory bytes: changed through the public iterator

After — the public object is a detached snapshot:

with this change
snapshot = next(memory.backers())[1]
snapshot type: bytes
snapshot mutation: unavailable
trusted internal reads: protected zero-copy path

@zardus

zardus commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Closing this. It is our own draft, it never drew a review, and having measured both sides: nothing outside cle triggers the problem it fixes, and it costs more than the description says on a path the description does not name.

Nobody writes through it

next(memory.backers())[1] hands out the loader's own bytearray, so writing into that object changes what later reads return. That is real. Outside cle, nobody does it.

Every occurrence of the identifier backers was enumerated in cle 0e77ade3, angr 87411a71 and angr-management aa843e5c — all 2,129 Python files, the whole population rather than a sample, because a caller cannot reach the yielded object without naming it and none of the three reaches it through getattr or a string key. That gives 14 call sites outside cle. Twelve only read: len(), a regex scan, a slice copy, bytes(...). Two could write, and neither does:

  • angr/engines/vex/lifter.py:319 hands the backer to libVEX as a raw pointer through pyvex.ffi.from_buffer. libVEX does not write guest code.
  • angr/storage/memory_mixins/paged_memory/page_backer_mixins.py:60 wraps the backer in a NotMemoryview, whose __setitem__ writes straight through, and shares it with a state page without copying. UltraPage.new_from_shared leaves that page at refcount 2, so the first write goes through acquire_unique(), which copies.

Both were then run, not just read. On tests/x86_64/libc.so.6, tests/i386/libc.so.6 and tests/x86_64/1after909 — taking a page that new_from_shared really did build from the loader's bytearray, 15, 7 and 1 shared pages respectively, and storing 16 bytes over it through state.memory — the loader's bytes come back identical. A VEX lift and 30 steps of execution on tests/x86_64/fauxware leave them identical too. The control in the same script is cle's own pack_word, which does write through a backer from backers() at memory.py:123, and it changes them, so the probe can see a write when there is one. On fauxware alone none of this would prove anything: a 2,676-byte backer is smaller than a page, so the sharing path never fires.

What it costs, including the half the description does not name

Head d341e928 against its own baseline a4fb8003, 50k calls, on fauxware with libraries. The machine was running other work, so single runs vary; these are the minimum of four runs per arm, and the ratios below moved between 2.7x and 6.9x run to run while the direction never did.

base head
pack_word 0.0682 s 0.3069 s 4.5x
pack 0.0567 s 0.2903 s 5.1x
store 2B 0.0592 s 0.1814 s 3.1x
load 4B 0.0596 s 0.1048 s 1.8x

One backers() walk over tests/x86_64/starling, 37.6 MB of backers, adds 35.5 MB of RSS — 115.3 to 150.8 MB — and keeps it for the life of the Clemory. On the baseline the same walk moves RSS by nothing at all, 113.0 to 113.0 MB.

The cost that matters is on the write path. Interleaving one pack_word with one next(backers(addr)) for 200 iterations on that binary: 0.0005 s on the baseline, 1.00 s on this head, about 2,000x, reproduced twice. Every write moves the cache key, so the next backers() copies the whole 33.9 MB backer again. backers() on its own is 0.0002 s against 0.0004 s — free, because it is served from the cache. It is writing and then reading that falls off. The description above says the hold was about reads.

The check that could have shown a regression and did not: an angr CFGFast on fauxware with libraries takes 62.2 s on the baseline and 56.7 s here, with the same 13,805 functions and maxRSS 825.8 against 832.2 MB. Load time does move, by less than the microbenchmarks suggest — fauxware 0.1247 to 0.1352 s, libc 0.1125 to 0.1265 s, starling 0.0435 to 0.0573 s. And angr writes to loader.memory ten times, all in SimLinux.configure_project, never in a loop with backers(), so the 2,000x is a trap left in the API rather than a cost anyone pays today. That is the argument for not paying 4-5x on cle's own relocation path now in order to close it.

The smaller versions do not work either

Handing out memoryview(...).toreadonly() instead of a copy breaks angr: three isinstance(backer, (bytes, bytearray[, mmap])) tests whose else-branch is raise TypeError(f"Unsupported backer type ...") — engines/vex/lifter.py:334, engines/pcode/lifter.py:1367, storage/memory_mixins/paged_memory/page_backer_mixins.py:100 — so a memoryview raises on every VEX lift, every p-code lift and every page initialisation, and breaks every released angr against a new cle. Dropping the cache is not a knob either: _initialize_page calls backers(addr) per page and both lifters call it per basic block, each taking the whole remaining backer, so uncached means a full-segment copy per page and per block.

What the work found that is worth keeping

Four defects on master, each now its own pull request rather than a rider on this one:

One thing here is dropped rather than moved, so it should be said plainly: ClemoryTranslator.pack and pack_word raise TypeError: Cannot access backers through address translation on master, because ClemoryBase.pack goes through backers() while store does not. This branch added a ClemoryTranslator.pack; closing it leaves that unfixed and unpublished. It is a small change on its own and nobody is holding it.

Two paths into an encrypted Mach-O region also stay open after #817, and closing either still needs a check inside cle/memory.py: Loader.memory_ro_view, which flattens the backers into plain bytearrays, and backers() itself, which is given an address and no size and so hands out the whole bytearray covering the region. Both are named on #817. Whoever closes them should widen those three isinstance sites in angr first and then hand out a read-only view, rather than build a snapshot cache.

@zardus zardus closed this Sep 6, 2026
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