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

Correcting X86 Imm Size #1657

Merged
merged 1 commit into from
Nov 11, 2021
Merged

Correcting X86 Imm Size #1657

merged 1 commit into from
Nov 11, 2021

Conversation

NicolasDerumigny
Copy link
Contributor

Correcting non-opened issue: x86 imm size is incorrectly reported (inversion of the first two columns of X86ImmSize.inc).

@gklingler
Copy link

Hi! Is any chance that this gets merged?

@kabeor
Copy link
Member

kabeor commented Nov 10, 2021

@NicolasDerumigny Hello, can you do a comparative test case so that we can see the problem more intuitively?

@NicolasDerumigny
Copy link
Contributor Author

I am unsure what you mean. Do you want me to implement some checks in tests/test_x86.c to ensure the width is well-reported?

@kabeor
Copy link
Member

kabeor commented Nov 11, 2021

looks no need. will merge soon. Thanks for your contribution

@kabeor kabeor merged commit 7e886c7 into capstone-engine:next Nov 11, 2021
@trufae
Copy link
Contributor

trufae commented Nov 13, 2021

This commit broke several tests in r2 (radareorg/radare2#19402)

I have compared the results with ghidra, binary ninja and IDA, and capstone is doing wrong after this patch. may be good to add some tests before merging things

@aeflores
Copy link
Contributor

yeah, this broke some test for us too.
As an example:

./cstool -d x64 4883e4f0
 0  48 83 e4 f0                                      and        rsp, 0xf0

This is after the change, but it is supposed to be (it is and rsp, -16):

 0  48 83 e4 f0                                      and        rsp, 0xfffffffffffffff0

this is X86_AND64ri8 with encoding size 1 and operand size 8. so: {1, 8, X86_AND64ri8}, rather than {8, 1, X86_AND64ri8},

@NicolasDerumigny
Copy link
Contributor Author

I am sorry for this bug, I am using capstone as a dependency and needed this patch so that the immediate size was correctly reported; I am trying to find exactly the example where this was needed and will reply as soon as I have found it.

@NicolasDerumigny
Copy link
Contributor Author

Okey, I believe this MR is utter garbage.

My issue was that cs_x86_op that are immediate seems to have a wrong size attribute (I am looking for the immediate size), and this inversion was providing me with accurate values. However, it is highly possible that I did not fully understood what I was doing, and then submit this buggy MR. I deeply apologize for the inconvenience and need to dig up further on this.

@kabeor
Copy link
Member

kabeor commented Nov 14, 2021

revert this first, hope you can fix it

@aquynh
Copy link
Collaborator

aquynh commented Nov 14, 2021 via email

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.

None yet

6 participants