Various fixes - #35
Conversation
Cover each fix from "Various fixes": - ld/st with concrete --src/--dst matches memory, not register (reverseame#30, reverseame#33) - generic memory address not resolved into an immediate (reverseame#33) - set_dst/set_src preserve the memory operand type (reverseame#33) - set_dst accepts immediate values (reverseame#33) - xchg src reported as a side effect (reverseame#31) - mov matches clc ; cmovae (valid Capstone mnemonics) (reverseame#32) - an explicitly written dst clears a stale clobber for later steps (reverseame#34) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review — LGTM
I reviewed the five fixes against issues #30-#34 and they are all correct. I added a commit with regression tests (test: add regression tests for #30-#34): they are discriminating (they fail on the previous code and pass with the fixes). Full suite: 102 passed (Python 3.13).
Per-issue verification:
- #31 (
gadget.py) — droppingself.srcfromexcludedmakesxchg dst, srcreportsrcas a side effect. It does not affectmov/loads (theresrcis not written). - #32 (
mov.yaml) — confirmed with Capstone: it emitscmovae/cmovb, notcmovc/cmovnc. The flag logic is also correct (clc+cmovaeandstc+cmovbalways move). - #33 (
operation.py) — all three errors:op_memis preserved inset_dst/set_src;set_dstaccepts immediates; and theself.is_reg()guard prevents a generic[src]from being resolved into an immediate. - #30 (
operation.py) — as a consequence of #33,--op ld --src <reg>now matchesmov <reg>, [reg]instead of the register-to-register move. - #34 (
ropchain.py) — writingdstexplicitly clears its clobber inside_effectedand restores it on backtrack (symmetric and correct).
Non-blocking note — st operations (mov [dst], src):
In a store, gad.dst is the address base register ([rax] -> rax), which is not written but consumed as an address. The #34 block treats it the same as a register dst and resets its clobber count, which could mask a previous gadget clobbering that address register. This is not a regression (a store's dst was never protected by the guard), but it would be better not to apply the refresh when dst is a memory base (e.g. distinguish the op_mem case, or rely on regs_write). This can be handled in a follow-up.
Approved.
The explicit-dst clobber refresh added in the ROP chain search (PR reverseame#35) zeroed the clobber counter of a gadget's dst whenever a step wrote it. For store operations (mov [dst], src), dst is the address base register, which is read as an address rather than written, so refreshing it could mask an earlier clobber and yield an invalid ROP chain. Guard the refresh on whether the gadget actually writes dst (Gadget.writes_reg), so it applies to register writes (mov, pop, xchg) but not to store address registers. Add a discriminating regression test. Closes reverseame#36 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #30, #31, #32, #33 and #34.
Summary
Fixes a set of related correctness bugs in operation matching, side-effect
tracking and the ROP chain search, and adds regression tests for each.
Fixes
operation.Operand#33 (memory operands)--op ld --src <reg>and--op st --dst <reg>matched register-to-register movs (and even immediate loads) instead of
memory accesses.
set_dst/set_srcnow preserve theop_memtype when aconcrete register is bound to a
[dst]/[src]placeholder, and the genericregister-to-immediate substitution in
is_equalis restricted to registeroperands, so a
[src]no longer matches an immediate.operation.Operand#33 (Operand fixes)set_dstnow accepts immediates likeset_src(
--dst 0x10produces anop_immoperand instead of being rejected), and ageneric memory address is no longer resolved into an immediate, so
mov rax, 0xcafeis not treated as a load.xchg dst, src#31 (xchg side effect)srcis no longer excluded from the computed sideeffects, so
xchg dst, srccorrectly reportssrcas clobbered.mnemonics
cmovb/cmovaeinstead of the invalidcmovc/cmovnc.dstfrom clobbered registers #34 (explicit dst clears clobber) A register written by a ROP chain step isno longer considered clobbered by earlier steps: the search clears the
destination register's clobber count during recursion and restores it on
backtrack.
Tests
Adds regression tests covering all five issues in
tests/test_operation.pyandtests/test_ropchain.py. Full suite passes locally (102 tests) and in CI onPython 3.11, 3.12 and 3.13.