Skip to content

Various fixes#35

Merged
ricardojrdez merged 2 commits into
reverseame:masterfrom
845977:master
Jul 3, 2026
Merged

Various fixes#35
ricardojrdez merged 2 commits into
reverseame:masterfrom
845977:master

Conversation

@845977

@845977 845977 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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

Tests

Adds regression tests covering all five issues in tests/test_operation.py and
tests/test_ropchain.py. Full suite passes locally (102 tests) and in CI on
Python 3.11, 3.12 and 3.13.

845977 and others added 2 commits July 2, 2026 20:57
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>

@ricardojrdez ricardojrdez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) — dropping self.src from excluded makes xchg dst, src report src as a side effect. It does not affect mov/loads (there src is not written).
  • #32 (mov.yaml) — confirmed with Capstone: it emits cmovae/cmovb, not cmovc/cmovnc. The flag logic is also correct (clc+cmovae and stc+cmovb always move).
  • #33 (operation.py) — all three errors: op_mem is preserved in set_dst/set_src; set_dst accepts immediates; and the self.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 matches mov <reg>, [reg] instead of the register-to-register move.
  • #34 (ropchain.py) — writing dst explicitly clears its clobber in side_effected and 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.

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.

ld/st operations match wrong gadgets when --src/--dst is specified

2 participants