Skip to content

AArch64: Replace vararg add_cs_detail by multiple concrete functions#2507

Merged
Rot127 merged 1 commit intocapstone-engine:nextfrom
thestr4ng3r:no-varargs-aarch64
Oct 22, 2024
Merged

AArch64: Replace vararg add_cs_detail by multiple concrete functions#2507
Rot127 merged 1 commit intocapstone-engine:nextfrom
thestr4ng3r:no-varargs-aarch64

Conversation

@thestr4ng3r
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) - current tests should be sufficient.

Detailed description

Fixes UB caused by various mismatches on how these arguments are passed and read. This became visible when running on PowerPC hosts with e.g. cstool -d aarch64 204862f8.
Apart from the UB fix, this is meant to be a pure refactor.

Test plan

see #2458 (comment)

Closing issues

Partially addresses #2458
Mips, PPC and Xtensa still have potentially problematic varargs.

@thestr4ng3r thestr4ng3r force-pushed the no-varargs-aarch64 branch 2 times, most recently from b60b6eb to ae46dbe Compare October 14, 2024 06:47
Copy link
Copy Markdown
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing it.
Please add the instruction from above as test in a file in tests/issues/.

@thestr4ng3r
Copy link
Copy Markdown
Contributor Author

Added. Keep in mind the bug in the original code does not manifest itself on current mainstream architectures.

@Rot127
Copy link
Copy Markdown
Collaborator

Rot127 commented Oct 17, 2024

Added. Keep in mind the bug in the original code does not manifest itself on current mainstream architectures.

Yes, I am aware. But I want to have it in case someone starts using Capstone on another weird machine. So just general test case coverage.

Copy link
Copy Markdown
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@kabeor kabeor left a comment

Choose a reason for hiding this comment

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

Cool, can you help to solve the conflict so I can merge it?

Fixes UB caused by various mismatches on how these arguments are passed
and read. This became visible when running on PowerPC hosts with e.g.
`cstool -d aarch64 204862f8`.
Apart from the UB fix, this is meant to be a pure refactor.

Partially addresses capstone-engine#2458
@thestr4ng3r
Copy link
Copy Markdown
Contributor Author

Sure, rebased now.

@Rot127 Rot127 merged commit 5026c2c into capstone-engine:next Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants