Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #3965 #3969

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Fix #3965 #3969

merged 1 commit into from
Nov 10, 2023

Conversation

Rot127
Copy link
Member

@Rot127 Rot127 commented Nov 9, 2023

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

This fixes a reachable assert() reached with Capstone v6. The operands used to calculate the jump target, used whatever was in the union of the operand value field.

This also means that the previous calculation of
jump targets were simply incorrect.

Since it never checks if the offset operand is a register or an immediate, it uses the enum value of the register (or something else) as offset in the first case.
Seems like we were lucky and MEMDISP() was always set to 0 in this case.

Test plan

One test added so far.
A new switch test is needed as well.

Closing issues

closes #3965

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

On a cursory look, you are right; old code looks wrong.

Copy link
Member

@thestr4ng3r thestr4ng3r left a comment

Choose a reason for hiding this comment

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

lgtm if tests pass

This fixes a reachable assert(). The operands used
to calculate the jump target, used whatever was in the
union of the operand value field.

This also means that the previous calculation of
jump targets were simply incorrect.
It nontheless lead to the right results because
MEMDISP() seemed to be 0 all the time.
@Rot127 Rot127 marked this pull request as ready for review November 9, 2023 20:55
@XVilka XVilka merged commit 839411c into rizinorg:dev Nov 10, 2023
43 checks passed
@Rot127 Rot127 deleted the arm-assert branch November 10, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants