Regression in SDL_ConvertAudio causing segmentation fault

[weirddan455]

I believe I've found a regression unless I'm just messing something silly up in how I'm calling SDL_ConvertAudio. I used git bisect and found that commit 111c3ad is where it broke.

I found the bug where I was doing conversion of real audio but was able to reproduce it in the following sample code with just a zero'd out audio buffer. cvt.len appears to be the culprit here. The number I used (16227964) happened to be the size of the real audio data I was working with. Replacing that with 20000000 works (no segfault).

In summary, the code below gives a segmentation fault. gdb shows it's coming from the function SDL_ResampleAudio (called from SDL_ConvertAudio. Before commit 111c3ad it works, afterwards it does not.

Operating System is Arch Linux if that matters.

#include <stdio.h>
#include <SDL2/SDL.h>

#define BUFFER_SIZE 209715200

int main(void)
{
    void *buffer = calloc(BUFFER_SIZE, 1);
    if (buffer == NULL) {
        puts("calloc failed");
        return 1;
    }
    if (SDL_Init(SDL_INIT_AUDIO) != 0) {
        puts("SDL_Init failed");
        return 1;
    }
    SDL_AudioCVT cvt;
    SDL_BuildAudioCVT(&cvt, AUDIO_S16LSB, 2, 32000, AUDIO_S16LSB, 2, 44100);
    cvt.len = 16227964;
    cvt.buf = buffer;
    if (cvt.len * cvt.len_mult > BUFFER_SIZE) {
        puts("Buffer too small");
        return 1;
    }
    if (SDL_ConvertAudio(&cvt) != 0) {
        puts("SDL_ConvertAudio failed");
    } else {
        puts("SDL_ConvertAudio Success");
    }
    return 0;
}

[icculus]

I'll check; I've got some weird buzzing in sdl12-compat in some cases too, and it would be nice if reverting this optimization fixes all this, honestly (the real win was moving from double to float, anyhow).

[oleg-derevenetz]

Yes, I'm currently investigating some weird crashes too (SDL 2.24.0 on Windows) like this:

Exception thrown at 0x00007FF89F8FA5D1 (SDL2.dll) in fheroes2.exe: 0xC0000005: Access violation reading location 0x00000249EDD1F000.

and I have call stacks that indicate that crashes are happening inside the SDL_ResampleAudio (the call chain is SDL_mixer's Mix_LoadWAV_RW -> SDL_ConvertAudio_REAL -> SDL_ResampleCVT -> SDL_ResampleAudio):

This happens sometimes when the audio file with rate 22050 was loaded and SDL attempts to resample it to the rate 48000.

P.S. What bothers me a bit here: with inrate 22050 and outrate 48000 the size of rpadding should be RESAMPLER_SAMPLES_PER_ZERO_CROSSING * chans, i.e. (1 << ((16 / 2) + 1)) * 2 = 1024 elements, while here:

            for (j = 0; (filterindex2 + (j * RESAMPLER_SAMPLES_PER_ZERO_CROSSING)) < RESAMPLER_FILTER_SIZE; j++) {
                const int jsamples = j * RESAMPLER_SAMPLES_PER_ZERO_CROSSING;
                const int srcframe = srcindex + 1 + j;
                /* !!! FIXME: we can bubble this conditional out of here by doing a post loop. */
                const float insample = (srcframe >= inframes) ? rpadding[((srcframe - inframes) * chans) + chan] : inbuf[(srcframe * chans) + chan];

srcindex is 461912 and inframes is 460051 at the moment of the crash, i.e. the value of the index inside rpadding is ((srcframe - inframes) * chans) + chan and cannot be less than 3722, which is way more than 1024 (unfortunately, I have just crash dump from the release build, so many variables were optimized out).

[weirddan455]

I can confirm it's reading outside the bounds of rpadding. The weird thing is changing typedef float ResampleFloatType; from float to double fixes the bug. I'm not sure why's that's the case. Floating point rounding error that's used for calculating the index? Or maybe there was a pre-existing bug in the logic that just happened to get exposed when changing the type.

From GDB:

Thread 1 "bug" hit Breakpoint 1, SDL_ResampleCVT (cvt=0x7fffffffe610, chans=2, format=33056) at /mnt/hdd/daniel/SDL/src/audio/SDL_audiocvt.c:454
454	    cvt->len_cvt = SDL_ResampleAudio(chans, inrate, outrate, padding, padding, src, srclen, dst, dstlen);
(gdb) p paddingsamples
$1 = 1024
(gdb) c
Continuing.

Thread 1 "bug" received signal SIGSEGV, Segmentation fault.
0x00007ffff7c35e77 in SDL_ResampleAudio (chans=2, inrate=32000, outrate=44100, lpadding=0x5555555a02e0, rpadding=0x5555555a02e0, inbuf=0x7fffeb130010, inbuflen=32455928, outbuf=0x7fffed023d08, outbuflen=97367784) at /mnt/hdd/daniel/SDL/src/audio/SDL_audiocvt.c:243
243	                const float insample = (srcframe >= inframes) ? rpadding[((srcframe - inframes) * chans) + chan] : inbuf[(srcframe * chans) + chan];
(gdb) p (srcframe - inframes) * chans
$2 = 28488
(gdb) p srcframe
$3 = 4071235
(gdb) p inframes
$4 = 4056991

[icculus]

So after a lot of agonizing, the fix is stupider than anticipated. :)

Basically we have a variable that does, once per iteration of the loop:

floatvalue += increment;

And with a double this works out because there's enough precision for this to not mess up, but on a float, the precision errors accumulate, so this continues to drift more for each iteration, eventually blowing up when there's a large enough buffer for these errors to pile up enough to cause us to overflow a buffer.

But there's no reason to accumulate errors, just multiply it by the loop counter each time instead:

floatvalue = increment * i;

This definitely fixes the test case in this bug, and likely other resampler bug reports. Valgrind shows no buffer overflows.

[sezero]

I guess that should go into the 2.24.x branch too?

[icculus]

I guess that should go into the 2.24.x branch too?

I'm hoping we're finishing 2.26.0 soon, but just in case, it should go in 2.24.x.

[sezero]

I'm hoping we're finishing 2.26.0 soon, but just in case, it should go in 2.24.x.

Done.