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

Fix multiple ubsan crashes #42

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix multiple ubsan crashes #42

wants to merge 4 commits into from

Conversation

antlarr
Copy link

@antlarr antlarr commented Mar 6, 2017

These commits fix #41, #31, #32, #34, #36, #37, #38, #39 and #40

antlarr added 3 commits March 6, 2017 13:57
When building the library with NDEBUG, asserts are eliminated
so it's better to always check that the number of coefficients
is inside the array range.

This fixes the 00191-audiofile-indexoob issue in mpruett#41
Check for multiplication overflow (using __builtin_mul_overflow
if available) in MSADPCM.cpp decodeSample and return an empty
decoded block if an error occurs.

This fixes the 00193-audiofile-signintoverflow-MSADPCM case of mpruett#41
Checks that a multiplication doesn't overflow when
calculating the buffer size, and if it overflows,
reduce the buffer size instead of failing.

This fixes the 00192-audiofile-signintoverflow-sfconvert case
in mpruett#41
@Mic92
Copy link

Mic92 commented Mar 21, 2017

What is the opinion of the maintainers on these patches?

@@ -281,6 +281,12 @@ status WAVEFile::parseFormat(const Tag &id, uint32_t size)

/* numCoefficients should be at least 7. */
assert(numCoefficients >= 7 && numCoefficients <= 255);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the assert statement still needed in this case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in case the code is built with DEBUG defined (as I guess @mpruett does) the assert would fail and I didn't want to change that behavior for the main developer. Also, in the general case (when DEBUG is not defined, as most distributions build this code), the assert is already a NOP.

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.

multiple ubsan crashes
2 participants