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.

Sign up for GitHub

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

Jump to bottom

UB on PPC because vargs are not casted. #2458

Open
Rot127 opened this issue Aug 26, 2024 · 3 comments
Open

UB on PPC because vargs are not casted. #2458

Rot127 opened this issue Aug 26, 2024 · 3 comments
Labels
Auto-Sync bug Something is not working as it should code-generation
Milestone
v6 - Beta

Comments

@Rot127
Copy link
Collaborator

Rot127 commented Aug 26, 2024 •
edited
Loading

Expected behavior

No OOB on PPC.

Actual behavior

The generator does not cast the varg arguments before passing them to add_cs_detail(). And sometimes it passes non-64bit values but unpacks them as uint64_t. Which is UB for some compilers (PPC apparently).
In general we should always cast to uint64_t values before passing them as vargs. The operand handler can cast them back.

Steps to reproduce the behavior

Compile Capstone for PPC.

Additional Logs, screenshots, source code, configuration dump, ...

Could be fixed by: #2135

cc @thestr4ng3r
@Rot127 Rot127 added bug Something is not working as it should code-generation Auto-Sync labels Aug 26, 2024
@Rot127 Rot127 added this to the v6 - Alpha milestone Aug 26, 2024
@capstone-engine capstone-engine deleted a comment Aug 26, 2024
@thestr4ng3r
Copy link
Contributor

This is UB, not necessarily anything OOB. I am able to reproduce it on Mac OS X 10.5 on ppc.
The safest solution would be to not use varargs at all, so the compiler can ensure that types are correct.

@Rot127
Copy link
Collaborator Author

Rot127 commented Aug 26, 2024

Ah, yes. I should think while writing. Added the relevant issue as well.

@Rot127 Rot127 changed the title OOB on PPC because vargs are not casted. UB on PPC because vargs are not casted. Aug 27, 2024
@Rot127 Rot127 modified the milestones: v6 - Alpha, v6 - Beta Sep 9, 2024
@thestr4ng3r
Copy link
Contributor

Reproducer:

xserve1:capstone florian$ uname -pv
Darwin Kernel Version 9.8.0: Wed Jul 15 16:57:01 PDT 2009; root:xnu-1228.15.4~1/RELEASE_PPC powerpc
xserve1:capstone florian$ cstool -d aarch64 204862f8
Assertion failed: (0 && "Extender not handled\n"), function AArch64_set_detail_shift_ext, file /Users/florian/dev/capstone/arch/AArch64/AArch64Mapping.c, line 2703.
Abort trap

thestr4ng3r added a commit to thestr4ng3r/capstone that referenced this issue Oct 13, 2024
@thestr4ng3r
AArch64: Replace vararg add_cs_detail by multiple concrete functions
0b820f8 
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 added a commit to thestr4ng3r/capstone that referenced this issue Oct 13, 2024
@thestr4ng3r
AArch64: Replace vararg add_cs_detail by multiple concrete functions
57a9daf 
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 thestr4ng3r mentioned this issue Oct 13, 2024
Merged
2 tasks
thestr4ng3r added a commit to thestr4ng3r/capstone that referenced this issue Oct 13, 2024
@thestr4ng3r
AArch64: Replace vararg add_cs_detail by multiple concrete functions
b60b6eb 
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 added a commit to thestr4ng3r/capstone that referenced this issue Oct 14, 2024
@thestr4ng3r
AArch64: Replace vararg add_cs_detail by multiple concrete functions
ae46dbe 
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 added a commit to thestr4ng3r/capstone that referenced this issue Oct 16, 2024
@thestr4ng3r
AArch64: Replace vararg add_cs_detail by multiple concrete functions
2d15c75 
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 added a commit to thestr4ng3r/capstone that referenced this issue Oct 21, 2024
@thestr4ng3r
AArch64: Replace vararg add_cs_detail by multiple concrete functions
1d13a12 
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
kabeor added a commit that referenced this issue Nov 24, 2024
@kabeor @Rot127
@thestr4ng3r @imbillow @AndrewQuijano @Antelox
Update to Version 6.0.0-Alpha2 (#2554)
66b0843 
* Update changelog for V6.0.0-Alpha1 (#2493)

* update version to v6-alpha1

* update bindings const values

* Update changelog for V6.0.0-Alpha1

* Remove irrelevant changes. (#2495)

* Fixing UB santizer, `LITBASE` and assert errors. (#2499)

* Update labeler with Xtensa and v6 files. (#2500)

* Add hard asserts to all SStream functions and memset MCInst. (#2501)

* Only trigger on released action. (#2497)

* Fix cstest build with Ninja (#2506)

* Tricore EA calculation (#2504)

* Update libcyaml dependency in cstest to 1.4.2 (#2508)

* AArch64: Replace vararg add_cs_detail by multiple concrete functions

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 #2458

* xtensa: Fix Branch Target (#2516)

* xtensa: Fix Branch Target

* auto-sync: fix byte pattern

* xtensa: add branch insn tests

* Revert "auto-sync: fix byte pattern"

This reverts commit cf8e870.

* Fix #2509. (#2510)

Compatibility headers should always include the header in the same dir.

* Fix stringop-truncation warning some compilers raise. (#2522)

* Add CC and VAS compatibility macros (#2525)

* Fix endianess issue during assignment. (#2528)

* This time actually fix big endian issue. (#2530)

* tricore: fixes #2474 (#2523)

* tricore: fix auto-sync tricore

* tricore: fixes TriCoreGenCSMappingInsnName.inc

* tricore: fixes

* tricore: try fix ld.a SC

* tricore: fixes all

* Add TriCore to .github/workflows/auto-sync.yaml

* Add TriCore details tests(a15, d15, a10|sp)

* Change CI to create Debian Package to Release (#2521)

* Updating CI to create Debian package and version is assigned by tag
version. Also updating release CI to not use end-of-life workflows

* Clear up usage of static libraries.

- Python bindings only use the dynamic lib. But built and copied the static ones sometimes nonetheless.
- Add toggles to build only static, static/dyn or only dynamic.

---------

Co-authored-by: Rot127 <unisono@quyllur.org>

* Rename build arguments: (#2534)

- BUILD_SHARED_LIBS -> CAPSTONE_BUILD_SHARED_LIBS
- BUILD_STATIC_LIBS -> CAPSTONE_BUILD_STATIC_LIBS
- BUILD_STATIC_LIBS -> CAPSTONE_BUILD_STATIC_MSVC_RUNTIME

* xtensa: update to espressif/llvm-project (#2533)

* fix coverity (#2546)

- cid 514642

- cid 514643

- cid 514644

- cid 514645

* Move debian package generation to a dispatch only workflow (#2543)

* Move deb package gen files int package/deb

* Fix basename check

* Make debian package generation dispatch only

* Python package building rework (#2538)

* - Refactored setup.py to remove hacks regarding packaging of wheels for different platforms, improve and cleanup the code
- Updated README.txt
- Removed old Makefile and build_wheel.sh scripts
- Created a new workflow that takes care of building and testing python packages for different platforms/architectures/python versions

* Added SPDX headers to the setup.py

* - cstest_py: Fixed positional argument since it doesn't accept a `required` flag. It turns to have a mandatory tests folder path
- integration_tests.py: Use pathlib to determine the required path
- GitHub action: Simplified the tests execution command

* GitHub Actions: Run python 3.8 (lowest) and 3.13 (current highest) for native runners only during testings and the rest during tag release

* GitHub Action:
- Fixed the cibw_build matrix element
- Added a step to prepare artifact name

* GitHub Action: Added run_tests.py script to run all tests during CI workflow

* - Added SPDX headers to the run_tests.py script and to the build-wheels-publish.yml workflow file
- Minor fixes to the workflow as pointed out in the PR review
- Updated MANIFEST.in to reflect the actual libraries built during python wheel creation process
- Use subprocess.run in place of os.system in run_tests.py script

* GitHub Action:
- Run qemu step only if non-native Linux runner
- Added arch:universal2 matrix element for macos-latest runner

* Python bindings: Refreshed the list of files needed to be copied for sdist archive

* GitHub Action: Commented out arch:x86 matrix elements

* GitHub Action: Run qemu step only if non-native Linux runner

* GitHub Action: Minor fixes

* Python bindings: Added missing .in pattern when collecting src files for sdist archive

* Auto-Sync reproducability + ARM update (#2532)

* fix xtensa DecodeMR23RegisterClass and add tests for MAC16 instru… (#2551)

* fix xtensa `DecodeMR23RegisterClass` and add tests for `MAC16` instructions

* revert

* Prepare for update (#2552)

* Bindings(chore): Fix DeprecationWarning

* Version(upgrade): update bindings const

* Fix(chore): Fix ARMCC_Invalid is not defined

* Update Changelog Version to 6.0.0-Alpha2 (#2553)

* Bindings(chore): Fix DeprecationWarning

* Version(upgrade): update bindings const

* Fix(chore): Fix ARMCC_Invalid is not defined

* Changelog: Update to version 6.0.0-Alpha2

---------

Co-authored-by: Rot127 <45763064+Rot127@users.noreply.github.com>
Co-authored-by: Florian Märkl <info@florianmaerkl.de>
Co-authored-by: billow <billow.fun@gmail.com>
Co-authored-by: Andrew <afq2101@columbia.edu>
Co-authored-by: Rot127 <unisono@quyllur.org>
Co-authored-by: @Antelox <anteloxrce@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Auto-Sync bug Something is not working as it should code-generation
Projects
None yet
Milestone
v6 - Beta
Development

No branches or pull requests
2 participants
@thestr4ng3r @Rot127