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.
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.