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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c48e4c6
Always check the number of coefficients
antlarr Mar 6, 2017
beacc44
Check for multiplication overflow in MSADPCM decodeSample
antlarr Mar 6, 2017
7d65f89
Check for multiplication overflow in sfconvert
antlarr Mar 6, 2017
ce536d7
Fix signature of multiplyCheckOverflow. It returns a bool, not an int
antlarr Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions libaudiofile/WAVE.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if (numCoefficients < 7 || numCoefficients > 255)
{
_af_error(AF_BAD_HEADER,
"Bad number of coefficients");
return AF_FAIL;
}

m_msadpcmNumCoefficients = numCoefficients;

Expand Down
5 changes: 3 additions & 2 deletions libaudiofile/modules/BlockCodec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ void BlockCodec::runPull()
// Decompress into m_outChunk.
for (int i=0; i<blocksRead; i++)
{
decodeBlock(static_cast<const uint8_t *>(m_inChunk->buffer) + i * m_bytesPerPacket,
static_cast<int16_t *>(m_outChunk->buffer) + i * m_framesPerPacket * m_track->f.channelCount);
if (decodeBlock(static_cast<const uint8_t *>(m_inChunk->buffer) + i * m_bytesPerPacket,
static_cast<int16_t *>(m_outChunk->buffer) + i * m_framesPerPacket * m_track->f.channelCount)==0)
break;

framesRead += m_framesPerPacket;
}
Expand Down
47 changes: 43 additions & 4 deletions libaudiofile/modules/MSADPCM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,24 +101,60 @@ static const int16_t adaptationTable[] =
768, 614, 512, 409, 307, 230, 230, 230
};

int firstBitSet(int x)
{
int position=0;
while (x!=0)
{
x>>=1;
++position;
}
return position;
}

#ifndef __has_builtin
#define __has_builtin(x) 0
#endif

bool multiplyCheckOverflow(int a, int b, int *result)
{
#if (defined __GNUC__ && __GNUC__ >= 5) || ( __clang__ && __has_builtin(__builtin_mul_overflow))
return __builtin_mul_overflow(a, b, result);
#else
if (firstBitSet(a)+firstBitSet(b)>31) // int is signed, so we can't use 32 bits
return true;
*result = a * b;
return false;
#endif
}


// Compute a linear PCM value from the given differential coded value.
static int16_t decodeSample(ms_adpcm_state &state,
uint8_t code, const int16_t *coefficient)
uint8_t code, const int16_t *coefficient, bool *ok=NULL)
{
int linearSample = (state.sample1 * coefficient[0] +
state.sample2 * coefficient[1]) >> 8;
int delta;

linearSample += ((code & 0x08) ? (code - 0x10) : code) * state.delta;

linearSample = clamp(linearSample, MIN_INT16, MAX_INT16);

int delta = (state.delta * adaptationTable[code]) >> 8;
if (multiplyCheckOverflow(state.delta, adaptationTable[code], &delta))
{
if (ok) *ok=false;
_af_error(AF_BAD_COMPRESSION, "Error decoding sample");
return 0;
}
delta >>= 8;
if (delta < 16)
delta = 16;

state.delta = delta;
state.sample2 = state.sample1;
state.sample1 = linearSample;
if (ok) *ok=true;

return static_cast<int16_t>(linearSample);
}
Expand Down Expand Up @@ -212,13 +248,16 @@ int MSADPCM::decodeBlock(const uint8_t *encoded, int16_t *decoded)
{
uint8_t code;
int16_t newSample;
bool ok;

code = *encoded >> 4;
newSample = decodeSample(*state[0], code, coefficient[0]);
newSample = decodeSample(*state[0], code, coefficient[0], &ok);
if (!ok) return 0;
*decoded++ = newSample;

code = *encoded & 0x0f;
newSample = decodeSample(*state[1], code, coefficient[1]);
newSample = decodeSample(*state[1], code, coefficient[1], &ok);
if (!ok) return 0;
*decoded++ = newSample;

encoded++;
Expand Down
34 changes: 32 additions & 2 deletions sfcommands/sfconvert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,33 @@ void printusage (void);
void usageerror (void);
bool copyaudiodata (AFfilehandle infile, AFfilehandle outfile, int trackid);

int firstBitSet(int x)
{
int position=0;
while (x!=0)
{
x>>=1;
++position;
}
return position;
}

#ifndef __has_builtin
#define __has_builtin(x) 0
#endif

bool multiplyCheckOverflow(int a, int b, int *result)
{
#if (defined __GNUC__ && __GNUC__ >= 5) || ( __clang__ && __has_builtin(__builtin_mul_overflow))
return __builtin_mul_overflow(a, b, result);
#else
if (firstBitSet(a)+firstBitSet(b)>31) // int is signed, so we can't use 32 bits
return true;
*result = a * b;
return false;
#endif
}

int main (int argc, char **argv)
{
if (argc == 2)
Expand Down Expand Up @@ -323,8 +350,11 @@ bool copyaudiodata (AFfilehandle infile, AFfilehandle outfile, int trackid)
{
int frameSize = afGetVirtualFrameSize(infile, trackid, 1);

const int kBufferFrameCount = 65536;
void *buffer = malloc(kBufferFrameCount * frameSize);
int kBufferFrameCount = 65536;
int bufferSize;
while (multiplyCheckOverflow(kBufferFrameCount, frameSize, &bufferSize))
kBufferFrameCount /= 2;
void *buffer = malloc(bufferSize);

AFframecount totalFrames = afGetFrameCount(infile, AF_DEFAULT_TRACK);
AFframecount totalFramesWritten = 0;
Expand Down