From 0e10cd2ae807afa777a1ee24faa971f29a03e7b4 Mon Sep 17 00:00:00 2001 From: "5684185+vsariola@users.noreply.github.com" <5684185+vsariola@users.noreply.github.com> Date: Sun, 22 Sep 2024 09:04:47 +0300 Subject: [PATCH] fix(amd64-386): sample oscillator hard crash The sample-based oscillators converted the samplepos to an integer and did samplepos < loop_end comparison to check if we are past looping. Unfortunately, the < comparison was done in signed math. Normally, this should never happen, but if the x87 FPU stack overflowed exactly at right position, we then got 0x80000000 in samplepos, which is equal to -2147483648. Thus, we considered that sample is not looping and read the sample table at position -2147483648, well out of bound. TL;DR changing jl to jb makes sure we always wrap within to sample table, no matter what. Fixes #149. --- CHANGELOG.md | 3 ++ tests/CMakeLists.txt | 5 ++ tests/test_oscillator_crash.c | 55 +++++++++++++++++++++ vm/compiler/templates/amd64-386/sources.asm | 2 +- 4 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/test_oscillator_crash.c diff --git a/CHANGELOG.md b/CHANGELOG.md index ec13bd9..14aeb32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). the command line tools. ### Fixed +- Sample-based oscillators could hard crash if a x87 stack overflow happened + when calculating the current position in the sample ([#149][i149]) - Numeric updown widget calculated dp-to-px conversion incorrectly, resulting in wrong scaling ([#150][i150]) - Empty patch should not crash the native synth ([#148][i148]) @@ -215,4 +217,5 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). [i145]: https://github.com/vsariola/sointu/issues/145 [i146]: https://github.com/vsariola/sointu/issues/146 [i148]: https://github.com/vsariola/sointu/issues/148 +[i149]: https://github.com/vsariola/sointu/issues/149 [i150]: https://github.com/vsariola/sointu/issues/150 diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 268e160..15904d0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -181,3 +181,8 @@ target_compile_definitions(test_render_samples PUBLIC TEST_HEADER="test_render_s add_executable(test_render_samples_api test_render_samples_api.c) target_link_libraries(test_render_samples_api ${STATICLIB}) add_test(test_render_samples_api test_render_samples_api) + +add_executable(test_oscillator_crash test_oscillator_crash.c) +target_link_libraries(test_oscillator_crash ${STATICLIB}) +add_test(test_oscillator_crash test_oscillator_crash) + diff --git a/tests/test_oscillator_crash.c b/tests/test_oscillator_crash.c new file mode 100644 index 0000000..ebe319b --- /dev/null +++ b/tests/test_oscillator_crash.c @@ -0,0 +1,55 @@ +#include +#include +#include +#include +#include + +#define BPM 100 +#define SAMPLE_RATE 44100 +#define LENGTH_IN_ROWS 16 +#define SAMPLES_PER_ROW SAMPLE_RATE * 4 * 60 / (BPM * 16) +const int su_max_samples = SAMPLES_PER_ROW * LENGTH_IN_ROWS; + +int main(int argc, char* argv[]) +{ + Synth* synth; + float* buffer; + // The patch is invalid and overflows the stack. This should still exit cleanly, but used to hard crash. + // See: https://github.com/vsariola/sointu/issues/149 + const unsigned char opcodes[] = { SU_OSCILLATOR_ID + 1, // STEREO + SU_ADVANCE_ID }; + const unsigned char operands[] = { 69, 74, 0, 0, 82, 128, 128 }; + int errcode; + int time; + int samples; + int totalrendered; + int retval; + // initialize Synth + synth = (Synth*)malloc(sizeof(Synth)); + memset(synth, 0, sizeof(Synth)); + memcpy(synth->Opcodes, opcodes, sizeof(opcodes)); + memcpy(synth->Operands, operands, sizeof(operands)); + synth->NumVoices = 3; + synth->Polyphony = 6; + synth->RandSeed = 1; + synth->SampleOffsets[0].Start = 91507; + synth->SampleOffsets[0].LoopStart = 5448; + synth->SampleOffsets[0].LoopLength = 563; + // initialize Buffer + buffer = (float*)malloc(2 * sizeof(float) * su_max_samples); + // triger first voice + synth->SynthWrk.Voices[0].Note = 64; + synth->SynthWrk.Voices[0].Sustain = 1; + totalrendered = 0; + samples = su_max_samples; + time = INT32_MAX; + retval = 0; + errcode = su_render(synth, buffer, &samples, &time); + if (errcode != 0x1041) { + retval = 1; + printf("su_render should have return errcode 0x1401, got 0x%08x\n", errcode); + } + free(synth); + free(buffer); + return retval; +} diff --git a/vm/compiler/templates/amd64-386/sources.asm b/vm/compiler/templates/amd64-386/sources.asm index 872f82a..9611c03 100644 --- a/vm/compiler/templates/amd64-386/sources.asm +++ b/vm/compiler/templates/amd64-386/sources.asm @@ -338,7 +338,7 @@ su_oscillat_gate_bit: ; stack: 0/1, let's call it x pop {{.DX}} ; edx is now the sample number movzx ebx, word [{{.DI}} + 4] ; ecx = loopstart sub edx, ebx ; if sample number < loop start - jl su_oscillat_sample_not_looping ; then we're not looping yet + jb su_oscillat_sample_not_looping ; then we're not looping yet mov eax, edx ; eax = sample number movzx ecx, word [{{.DI}} + 6] ; edi is now the loop length xor edx, edx ; div wants edx to be empty