Skip to content

m68k: fix undefined abs() of INT32_MIN displacement in printer - #2990

Closed
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:m68k-abs-int-min
Closed

m68k: fix undefined abs() of INT32_MIN displacement in printer#2990
Samin061 wants to merge 1 commit into
capstone-engine:nextfrom
Samin061:m68k-abs-int-min

Conversation

@Samin061

@Samin061 Samin061 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

printBaseDisp and printMemIndirect print a 32-bit displacement by calling abs() on op->mem.in_disp / op->mem.out_disp, but those fields are filled from read_imm_32 in the disassembler, so a crafted M68K memory-indirect operand can set them to INT32_MIN, where abs() negates a value with no representable positive counterpart and the behavior is undefined. The magnitude now goes through a small unsigned-negation helper so it stays well-defined at the three 32-bit sites; the sign is already printed separately, so the output is unchanged. The int16_t disp sites cannot reach INT32_MIN and are left alone.

Test plan

Building with -fsanitize=undefined and running test_poc aborts at M68KInstPrinter.c on cs_disasm of the new test_ub_abs_m68k_disp input before the change and exits cleanly after. That case disassembles a full-format memory-indirect move.w whose outer displacement is 0x80000000.

Closing issues

@github-actions github-actions Bot added the M68K Arch label Jul 8, 2026
@Rot127

Rot127 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

There is literally an util function for it.
Closing it for the same reason as #2989 (comment)
As said, feel free to fix the issue again after properly evaluating the code change.

@Rot127 Rot127 closed this Jul 8, 2026
@Samin061

Samin061 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Took another look at this. printInt32HexOffset/printInt16HexOffset do handle the same UB, but they emit the offset as +0x.../-0x..., which changes the M68K $ syntax and the existing m68k.yaml detail case (e.g. ([$7fffffff, a0], d0.w, $12345678) would turn into ([+0x7fffffff, ...], ..., +0x12345678)). So they aren't a drop-in here.

To keep the disassembly output byte-for-byte identical I dropped the local helper and used the same well-defined unsigned negation those utils rely on internally, inline at the three 32-bit sites. I also added a cstest case in tests/details/m68k.yaml asserting the INT32_MIN outer displacement prints as -$80000000, and checked it with cstool plus a clean UBSan run.

I pushed the update to the branch, but GitHub won't let me reopen since it was force-pushed. If you'd rather standardize on the 0x offset form I'm happy to switch to the util and update the affected M68K tests instead. Let me know how you'd like to proceed (reopen on your side, or a fresh PR).

@Rot127

Rot127 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Then please add another function to SStream which just prints in decimal, add unit tests and use it here.
The changes here are prone to introduce code duplication and are bad software design.

@Samin061

Copy link
Copy Markdown
Contributor Author

Added printInt32DispHex to SStream. It formats a signed 32-bit displacement as the Motorola $hex the M68K printer already emits ($abcd / -$abcd) and builds the INT32_MIN magnitude without negating it, so the inline abs() and the local helper are both gone. The three 32-bit sites in printBaseDisp/printMemIndirect call it now, the disassembly is byte-for-byte identical, and there's a unit test in tests/unit/sstream.c covering INT32_MIN. Clean under -fsanitize=undefined,integer.

I kept the helper on $hex rather than decimal because M68K uses $ for displacements (e.g. the existing ([$7fffffff, a0], d0.w, $12345678) detail case), so a decimal base would change all of those. If you'd rather standardize on decimal I'll switch the helper and update the M68K tests.

GitHub won't let me reopen this after the earlier force-push, so the branch has the update but the PR stays closed. Happy to reopen on your side, or I can put it up as a fresh PR, whichever you prefer.

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

Labels

M68K Arch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants