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

PPC: fix out of bound memory access #1913

Merged
merged 1 commit into from Sep 8, 2022

Conversation

hamarituc
Copy link
Contributor

Please review the patch carefully. It at least fixes an out of bound memory access. But I suppose the former as well as the new code doesn't produce the intended behavior. The variable name may contain condition register names like cr0, cr1, ..., but the code seems to be meant for condition register bits cr0eq, cr0gt, cr0lt, cr0un, ...

closes #1912

@kabeor
Copy link
Member

kabeor commented Aug 29, 2022

Thanks for your contribution, plz add instruction test at https://github.com/capstone-engine/capstone/blob/next/suite/cstest/issues.cs

@hamarituc
Copy link
Contributor Author

Thanks for your contribution, plz add instruction test at https://github.com/capstone-engine/capstone/blob/next/suite/cstest/issues.cs

Thanks for the remark.

Is there a way to set the CS_OPT_NOREGNAME option in the test? I tried with

!# issue 1912 PPC register name
!# CS_ARCH_PPC, CS_MODE_BIG_ENDIAN, None
0x2d,0x03,0x00,0x80 == cmpwi cr2, r3, 0x80

!# issue 1912 PPC no register name
!# CS_ARCH_PPC, CS_MODE_BIG_ENDIAN, CS_OPT_NOREGNAME
0x2d,0x03,0x00,0x80 == cmpwi 2, 3, 0x80

but the options are just comments. Without this possibility a test wouldn't make much sense, because there is no way to reproduce the failure conditions.

Even then, the test would not be accurate in detecting a regression as this patch fixes a memory corruption. This is not a deterministic behavior. Whether the test fails depends on the C library implementation, the order of memory allocation and releases, library versions etc. It makes more sense to identify such issues by running the regular test suite with -fsanitize=address. The issue was discovered that way by running the provided tests on a PowerPC machine.

@kabeor
Copy link
Member

kabeor commented Sep 7, 2022

Is there a way to set the CS_OPT_NOREGNAME option in the test? I tried with

Acutally yes, there is a flag names CS_OPT_SYNTAX_NOREGNAME, refer here

@hamarituc
Copy link
Contributor Author

Is there a way to set the CS_OPT_NOREGNAME option in the test? I tried with

Acutally yes, there is a flag names CS_OPT_SYNTAX_NOREGNAME, refer here

Thanks for the hint!

@kabeor
Copy link
Member

kabeor commented Sep 8, 2022

Great, thanks for your contribution!

@kabeor kabeor merged commit afb5575 into capstone-engine:next Sep 8, 2022
@hamarituc hamarituc deleted the ppc-buffer-underrun branch September 8, 2022 06:07
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

2 participants