Skip to content

Fixed runtime error and uninitialized variable usage #46

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

Closed
wants to merge 2 commits into from
Closed

Fixed runtime error and uninitialized variable usage #46

wants to merge 2 commits into from

Conversation

wargio
Copy link

@wargio wargio commented Jun 27, 2022

I have embedded libmspack in rizin and we had these issues raised by our CI

+../subprojects/libmspack/libmspack/mspack/cabd.c:1403:66: runtime error: left shift of 156 by 24 places cannot be represented in type 'int'
../subprojects/libmspack/libmspack/mspack/chmd.c: In function ‘chmd_extract’:
../subprojects/libmspack/libmspack/mspack/chmd.c:1139:10: error: ‘length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1139 |   length -= self->d->offset;
     |          ^~
../subprojects/libmspack/libmspack/mspack/chmd.c:1042:9: note: ‘length’ was declared here
1042 |   off_t length, offset;
     |         ^~~~~~
cc1: all warnings being treated as errors

If interested i have a working meson configuration for the project.

@kyz
Copy link
Owner

kyz commented Feb 4, 2023

Thank you for incorporating libmspack into the rizin project, and thank you for the patch.

I made both changes directly to the source, rather than merge it.

  • There are lots of places where UBSan complains and I fixed all of them in a6b8a22
  • The warning is a false positive, I have a full walkthrough of the flow here: Compilier warnings with 1.9.1 #36 (comment)
  • The warning is quelled in 0d13d86 (just before gcc's analysis fails, rather than turn off analysis completely by initialising in the declaration)

As far as build systems are concerned, libmspack will continue to use autotools as it allows supporting a wide range of esoteric systems in addition to modern Linux/BSDs, and there is also another large merge request in progress (#32) to introduce the CMake build system, mainly for better support on Windows. But thank you for the offer.

@kyz kyz closed this Feb 4, 2023
@wargio
Copy link
Author

wargio commented Feb 5, 2023

awesome. so it's just up to me to update to the latest commit. thanks for the effort.

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.

2 participants