From df7ceb848c1918ce818db37307d8e950fe57dc45 Mon Sep 17 00:00:00 2001 From: austek <13117393+austek@users.noreply.github.com> Date: Sun, 23 Aug 2026 09:23:06 +0100 Subject: [PATCH] Fix integer overflow in insertPitchPeriod/enlargeOutputBufferIfNeeded period + newSamples is computed correctly in long in insertPitchPeriod, but was truncated to int when passed into enlargeOutputBufferIfNeeded's int parameter. Inside that function, the int addition numOutputSamples + numSamples and the buffer growth calculation can also signed-overflow, both leading to a too-small allocation followed by writes sized for the original, larger sample count. Guard both: insertPitchPeriod now checks the long sum against INT_MAX before truncating, and enlargeOutputBufferIfNeeded checks its own addition for overflow before applying it, returning 0 through the existing failure path in both cases instead of invoking undefined behavior. Adds tests/overflow_test.c, a white-box test that calls both functions directly (they are no longer static, for this reason) with values chosen to hit the overflow guards. Ported from waywardgeek/sonic#62, which fixes the same bug (also present here, byte-identical insertPitchPeriod/enlargeOutputBufferIfNeeded) upstream. --- sonic.c | 37 ++++++++++++++---- tests/Makefile | 3 +- tests/overflow_test.c | 88 +++++++++++++++++++++++++++++++++++++++++++ tests/runtests.c | 3 ++ tests/tests.h | 3 ++ 5 files changed, 125 insertions(+), 9 deletions(-) create mode 100644 tests/overflow_test.c diff --git a/sonic.c b/sonic.c index 8e2f696..0223189 100644 --- a/sonic.c +++ b/sonic.c @@ -461,12 +461,23 @@ void sonicSetNumChannels(sonicStream stream, int numChannels) { allocateStreamBuffers(stream, stream->sampleRate, numChannels); } -/* Enlarge the output buffer if needed. */ -static int enlargeOutputBufferIfNeeded(sonicStream stream, int numSamples) { +/* Enlarge the output buffer if needed. Not static: exercised directly by + the white-box overflow tests in tests/overflow_test.c. */ +int enlargeOutputBufferIfNeeded(sonicStream stream, int numSamples) { int outputBufferSize = stream->outputBufferSize; + int growth; - if (stream->numOutputSamples + numSamples > outputBufferSize) { - stream->outputBufferSize += (outputBufferSize >> 1) + numSamples; + if (numSamples < 0) { + return 0; + } + /* Written as a subtraction, not "numOutputSamples + numSamples", so the + comparison itself can't signed-overflow when numSamples is huge. */ + if (stream->numOutputSamples > outputBufferSize - numSamples) { + growth = outputBufferSize >> 1; + if (numSamples > INT_MAX - outputBufferSize - growth) { + return 0; + } + stream->outputBufferSize = outputBufferSize + growth + numSamples; stream->outputBuffer = (short*)sonicRealloc( stream->outputBuffer, outputBufferSize, stream->outputBufferSize, sizeof(short) * stream->numChannels); @@ -1040,10 +1051,13 @@ static int skipPitchPeriod(sonicStream stream, short* samples, float speed, return newSamples; } -/* Insert a pitch period, and determine how much input to copy directly. */ -static int insertPitchPeriod(sonicStream stream, short* samples, float speed, - int period) { +/* Insert a pitch period, and determine how much input to copy directly. Not + static: exercised directly by the white-box overflow tests in + tests/overflow_test.c. */ +int insertPitchPeriod(sonicStream stream, short* samples, float speed, + int period) { long newSamples; + long totalSamples; short* out; int numChannels = stream->numChannels; @@ -1052,7 +1066,14 @@ static int insertPitchPeriod(sonicStream stream, short* samples, float speed, } else { newSamples = period; } - if (!enlargeOutputBufferIfNeeded(stream, period + newSamples)) { + /* period + newSamples is computed in long above, but + enlargeOutputBufferIfNeeded takes an int; reject here instead of + silently truncating a huge period into a small/negative int. */ + totalSamples = (long)period + newSamples; + if (totalSamples < 0 || totalSamples > INT_MAX) { + return 0; + } + if (!enlargeOutputBufferIfNeeded(stream, (int)totalSamples)) { return 0; } out = stream->outputBuffer + stream->numOutputSamples * numChannels; diff --git a/tests/Makefile b/tests/Makefile index 9597190..028128f 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -16,7 +16,8 @@ CFLAGS=-Wall -Wno-unused-function -g -ansi -fPIC -pthread -I .. #CFLAGS += -Wall -Wno-unused-function -ansi -fPIC -pthread -I .. TEST_SRC = \ -input_clamping_test.c +input_clamping_test.c \ +overflow_test.c CC=gcc diff --git a/tests/overflow_test.c b/tests/overflow_test.c new file mode 100644 index 0000000..f585cb8 --- /dev/null +++ b/tests/overflow_test.c @@ -0,0 +1,88 @@ +/* Sonic library + Copyright 2026 + Bill Cox + This file is part of the Sonic Library. + + This file is licensed under the Apache 2.0 license. +*/ + +#include "sonic.h" + +#include +#include + +#define SAMPLE_RATE 44100 +#define NUM_CHANNELS 1 + +/* enlargeOutputBufferIfNeeded and insertPitchPeriod are not declared static + in sonic.c specifically so these white-box tests can call them directly, + without going through the public streaming API to reach the overflow + paths. They are not part of the public sonic.h contract. */ +int enlargeOutputBufferIfNeeded(sonicStream stream, int numSamples); +int insertPitchPeriod(sonicStream stream, short* samples, float speed, + int period); + +int sonicTestEnlargeOutputBufferRejectsOverflow(void) { + sonicStream stream; + int result; + + stream = sonicCreateStream(SAMPLE_RATE, NUM_CHANNELS); + if (stream == NULL) { + fprintf(stderr, "sonicCreateStream failed\n"); + return 0; + } + /* A request this large can't be satisfied; the guard must fail cleanly + instead of signed-overflowing the internal buffer size arithmetic. */ + result = enlargeOutputBufferIfNeeded(stream, INT_MAX); + sonicDestroyStream(stream); + if (result != 0) { + fprintf(stderr, + "enlargeOutputBufferIfNeeded should reject an overflowing request\n"); + return 0; + } + return 1; +} + +int sonicTestEnlargeOutputBufferAcceptsNormalRequest(void) { + sonicStream stream; + int result; + + stream = sonicCreateStream(SAMPLE_RATE, NUM_CHANNELS); + if (stream == NULL) { + fprintf(stderr, "sonicCreateStream failed\n"); + return 0; + } + result = enlargeOutputBufferIfNeeded(stream, 128); + sonicDestroyStream(stream); + if (result != 1) { + fprintf(stderr, + "enlargeOutputBufferIfNeeded should accept a normal request\n"); + return 0; + } + return 1; +} + +int sonicTestInsertPitchPeriodRejectsOverflow(void) { + sonicStream stream; + short dummy[1]; + int result; + + stream = sonicCreateStream(SAMPLE_RATE, NUM_CHANNELS); + if (stream == NULL) { + fprintf(stderr, "sonicCreateStream failed\n"); + return 0; + } + dummy[0] = 0; + /* period this large makes period + newSamples overflow INT_MAX before + the fix, which used to truncate silently at the + enlargeOutputBufferIfNeeded call boundary. The function must return + failure before touching samples, so a 1-element buffer is safe here. */ + result = insertPitchPeriod(stream, dummy, 1.0f, INT_MAX); + sonicDestroyStream(stream); + if (result != 0) { + fprintf(stderr, + "insertPitchPeriod should reject an overflowing period\n"); + return 0; + } + return 1; +} diff --git a/tests/runtests.c b/tests/runtests.c index a7ef2e9..c87223d 100644 --- a/tests/runtests.c +++ b/tests/runtests.c @@ -16,6 +16,9 @@ int main(int argc, char** argv) { assert(sonicTestInputClamping()); assert(sonicTestInputsDontCrash()); + assert(sonicTestEnlargeOutputBufferRejectsOverflow()); + assert(sonicTestEnlargeOutputBufferAcceptsNormalRequest()); + assert(sonicTestInsertPitchPeriodRejectsOverflow()); printf("All tests passed.\n"); return 0; } diff --git a/tests/tests.h b/tests/tests.h index 9f08d06..f34da4a 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -12,6 +12,9 @@ extern "C" { int sonicTestInputClamping(void); int sonicTestInputsDontCrash(void); +int sonicTestEnlargeOutputBufferRejectsOverflow(void); +int sonicTestEnlargeOutputBufferAcceptsNormalRequest(void); +int sonicTestInsertPitchPeriodRejectsOverflow(void); #ifdef __cplusplus }