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

[v5] python: Fix definition of capstone syntax value option constants #2240

Merged
merged 1 commit into from
Jan 18, 2024
Merged

[v5] python: Fix definition of capstone syntax value option constants #2240

merged 1 commit into from
Jan 18, 2024

Conversation

nmeum
Copy link

@nmeum nmeum commented Jan 9, 2024

The option values for capstone syntax bindings are currently out-of-sync with capstone.h on the v5 branch.

This causes Python code using the syntax options to not work correctly. For example, consider:

import capstone as c
md = c.Cs(c.CS_ARCH_X86, c.CS_MODE_32)
md.syntax = c.CS_OPT_SYNTAX_ATT

This code will presently error on the latest 5.0.1 release, it works on both the next branch and the 4.X releases.

The error message is:

capstone.CsError: Invalid option (CS_ERR_OPTION)

This is because commit 53eaeac broke the option values on the v5 branch as it caused capstone.h and bindings/python/capstone/__init__.py to define these values differently.

Compare

/// Runtime option value (associated with option type above)
typedef enum cs_opt_value {
CS_OPT_OFF = 0, ///< Turn OFF an option - default for CS_OPT_DETAIL, CS_OPT_SKIPDATA, CS_OPT_UNSIGNED.
CS_OPT_ON = 3, ///< Turn ON an option (CS_OPT_DETAIL, CS_OPT_SKIPDATA).
CS_OPT_SYNTAX_DEFAULT = 0, ///< Default asm syntax (CS_OPT_SYNTAX).
CS_OPT_SYNTAX_INTEL, ///< X86 Intel asm syntax - default on X86 (CS_OPT_SYNTAX).
CS_OPT_SYNTAX_ATT, ///< X86 ATT asm syntax (CS_OPT_SYNTAX).
CS_OPT_SYNTAX_NOREGNAME, ///< Prints register name with only number (CS_OPT_SYNTAX)
CS_OPT_SYNTAX_MASM, ///< X86 Intel Masm syntax (CS_OPT_SYNTAX).
CS_OPT_SYNTAX_MOTOROLA, ///< MOS65XX use $ as hex prefix
} cs_opt_value;

with

# Capstone syntax value
CS_OPT_SYNTAX_DEFAULT = 1 << 1 # Default assembly syntax of all platforms (CS_OPT_SYNTAX)
CS_OPT_SYNTAX_INTEL = 1 << 2 # Intel X86 asm syntax - default syntax on X86 (CS_OPT_SYNTAX, CS_ARCH_X86)
CS_OPT_SYNTAX_ATT = 1 << 3 # ATT asm syntax (CS_OPT_SYNTAX, CS_ARCH_X86)
CS_OPT_SYNTAX_NOREGNAME = 1 << 4 # Asm syntax prints register name with only number - (CS_OPT_SYNTAX, CS_ARCH_PPC, CS_ARCH_ARM)
CS_OPT_SYNTAX_MASM = 1 << 5 # MASM syntax (CS_OPT_SYNTAX, CS_ARCH_X86)
CS_OPT_SYNTAX_MOTOROLA = 1 << 6 # MOS65XX use $ as hex prefix
CS_OPT_SYNTAX_CS_REG_ALIAS = 1 << 7 # Prints common register alias which are not defined in LLVM (ARM: r9 = sb etc.)

This was discovered while debugging an angr test failure with Capstone 5.0.1.

@Rot127
Copy link
Collaborator

Rot127 commented Jan 9, 2024

@kabeor Be so kind and trigger the CI.

@Rot127
Copy link
Collaborator

Rot127 commented Jan 10, 2024

On a second look. I think it is better if the enum in capstone.h is changed to the shifted values. The values in v5 capstone.h seem weird. CS_OPT_ON is equivalent to CS_OPT_SYNTAX_NOREGNAME which doesn't make sense to me.

Besides, in next it is like __init__.py. So the update is better towards next so it doesn't deviate.

Would you agree?

@peace-maker
Copy link
Contributor

This is still the aftermath of #2097 getting backported completely into v5 while the second commit wasn't supposed to be backported since it aligns the Python bindings with the new constant values introduced with the ARM auto-sync merge.

There are several issues mentioning this #2145 #2144 and I've explained this here #2145 (comment) It seems the commit was never reverted, so this PR should revert more than only the SYNTAX constants to clean it up.

Changing such definitions in the core in minor version updates might cause trouble with other bindings, so I'd advocate to only fix the Python bindings and wait until v6 to change them.

@nmeum
Copy link
Author

nmeum commented Jan 12, 2024

So how do we get this fixed? Do you want me to update this PR to revert more of 53eaeac? Should I just revert all changes to the __init__.py file made in that commit?

@Rot127
Copy link
Collaborator

Rot127 commented Jan 13, 2024

On a shallow look (sorry, don't have too much time currently) reverting commit 2ed0906 is the way to go. @peace-maker Can you confirm?

The ARM auto-sync merge was not supposed to be backported to the v5
branch but was accidentally backported. For more information refer
to: #2240 (comment)

This reverts part of commit 53eaeac.
@nmeum
Copy link
Author

nmeum commented Jan 14, 2024

Can someone with push access just do that or do you want me to update the PR?

Also note: 2ed0906 is not a commit on the v5 branch. It seems to have been squashed into 53eaeac on the v5 branch as far as I can tell.

@nmeum
Copy link
Author

nmeum commented Jan 14, 2024

Just went ahead and updated the PR.

@Rot127
Copy link
Collaborator

Rot127 commented Jan 15, 2024

@kabeor Please trigger the workflows

Copy link
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.

LGTM. @peace-maker any more comments?

@nmeum
Copy link
Author

nmeum commented Jan 17, 2024

Ready to merge or do you need me to do anything else?

@kabeor
Copy link
Member

kabeor commented Jan 18, 2024

Looks good, thank you!

@kabeor kabeor merged commit f180e17 into capstone-engine:v5 Jan 18, 2024
6 checks passed
bell-sw pushed a commit to bell-sw/alpaquita-aports that referenced this pull request Jan 23, 2024
@tyjman
Copy link

tyjman commented Mar 21, 2024

Are there any plans on releasing a new version with this fix?

@Rot127
Copy link
Collaborator

Rot127 commented Mar 21, 2024

Will be part of v5.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants