SDL 2.0.6 causes crashes with small audio buffer sizes

[devnexen]

Similar issue than here

bradharding/doomretro#410

looking at i_sdlsound.c seems the same kind of change fixes the issue (with Doom II again).

@fragglet if you have a better insight since I m not a SDL wizard, would appreciate.

[fragglet]

Changed description to be more specific; if it is incorrect please fix.

[JNechaevsky]

Here's what is happening with latest Chocolate Doom build on Windows:

1. Download latest dev build.
2. Replace SDL.dll (2.0.5) from unpacked archive with newly released (2.0.6).
3. Game seems to be running fine, but there is no sound and music. No freezes and lockups, just no sound at all.

I was trying to recompile CD sources with updating SDL's development libraries to 2.0.6, but no luck.
I've also have no idea how to test this case on Debian, since I'm not very familiar with changing stable/testing package sources.

Also, @turol explained on IRC that this is seems to be a combination problem of new SDL + SDL_mixer, i.e., we need a new version of SDL_mixer which was not released yet.

[fragglet]

Filed a bug with SDL:

https://bugzilla.libsdl.org/show_bug.cgi?id=3858

[fragglet]

This has been resolved upstream by SDL devs. https://bugzilla.libsdl.org/show_bug.cgi?id=3858

[OrdinaryMagician]

Updated to 2.0.7 a moment ago. The crash is still happening, at least on Linux.

[ndrchvzz]

I can confirm it is still crashing on Arch Linux with SDL 2.0.7.

[fragglet]

Thanks for the feedback. I've reopened the SDL bug.

[devnexen]

@fragglet do you think it is up to SDL developers to fix or do you think chocolate-doom should, somehow, find a fix ?

[fragglet]

The fix from our end is to not ship prebuilt binaries containing SDL 2.0.6 or SDL 2.0.7. The rest is up to the SDL devs.

[ericwa]

I'm getting a crash with 4746d23 on macOS 10.13 and SDL 2.0.7 installed from homebrew.

I think there is a bug in chocolate doom: I added a printf to ExpandSoundData_SDL to check the size of the convertor.buf buffer, and it looks like the convertor.buf is not large enough; it needs to have room for at least convertor.len * convertor.len_mult bytes according to the docs at: https://wiki.libsdl.org/SDL_AudioCVT

$ git diff
diff --git a/src/i_sdlsound.c b/src/i_sdlsound.c
index 32653099..e3f07cbc 100644
--- a/src/i_sdlsound.c
+++ b/src/i_sdlsound.c
@@ -633,7 +633,8 @@ static boolean ExpandSoundData_SDL(sfxinfo_t *sfxinfo,
     {
         convertor.buf = chunk->abuf;
         convertor.len = length;
-        memcpy(convertor.buf, data, length);
+       printf("chunk->alen: %d. len*len_mult: %d\n", chunk->alen, convertor.len * convertor.len_mult);
+       memcpy(convertor.buf, data, length);
 
         SDL_ConvertAudio(&convertor);
     }

btw I configured with: CFLAGS=-fsanitize=address ./configure
here is the output when it crashes, which happens when I press Enter on "New Game":

$ ./src/chocolate-doom -iwad /Volumes/TOSHIBA\ EXT/steam/SteamApps/common/Ultimate\ Doom/base/DOOM.WAD 
                      Chocolate Doom 3.0.0-beta1
Z_Init: Init zone memory allocation daemon. 
zone memory: 0x10a86f800, 1000000 allocated for zone
Using /Users/ericwa/Library/Application Support/chocolate-doom/ for configuration and saves
V_Init: allocate screens.
M_LoadDefaults: Load system defaults.
saving config in /Users/ericwa/Library/Application Support/chocolate-doom/default.cfg
W_Init: Init WADfiles.
 adding /Volumes/TOSHIBA EXT/steam/SteamApps/common/Ultimate Doom/base/DOOM.WAD
===========================================================================
                           The Ultimate DOOM
===========================================================================
 Chocolate Doom is free software, covered by the GNU General Public
 License.  There is NO warranty; not even for MERCHANTABILITY or FITNESS
 FOR A PARTICULAR PURPOSE. You are welcome to change and distribute
 copies under certain conditions. See the source for more information.
===========================================================================
I_Init: Setting up machine state.
OPL_Init: Using driver 'SDL'.
NET_Init: Init network subsystem.
M_Init: Init miscellaneous info.
R_Init: Init DOOM refresh daemon - [..........................]
P_Init: Init Playloop state.
S_Init: Setting up sound.
D_CheckNetGame: Checking network game status.
startskill 2  deathmatch: 0  startmap: 1  startepisode: 1
player 1 of 1 (1 nodes)
Emulating the behavior of the 'Ultimate Doom' executable.
HU_Init: Setting up heads up display.
ST_Init: Init status bar.
chunk->alen: 90064. len*len_mult: 360256
ASAN:DEADLYSIGNAL
=================================================================
==24418==ERROR: AddressSanitizer: SEGV on unknown address 0x632000030000 (pc 0x0001071a1d38 bp 0x7ffee8e81190 sp 0x7ffee8e810a0 T0)
    #0 0x1071a1d37 in SDL_ResampleAudio (libSDL2-2.0.0.dylib:x86_64+0xbd37)
    #1 0x1071a1961 in SDL_ResampleCVT (libSDL2-2.0.0.dylib:x86_64+0xb961)
    #2 0x10719fc83 in SDL_ConvertAudio_REAL (libSDL2-2.0.0.dylib:x86_64+0x9c83)
    #3 0x106d98a6e in I_SDL_StartSound i_sdlsound.c:639
    #4 0x106e83917 in S_StartSound s_sound.c:528
    #5 0x106e07553 in M_Responder m_menu.c:1830
    #6 0x106de01cc in D_ProcessEvents d_main.c:149
    #7 0x106d88820 in BuildNewTic d_loop.c:148
    #8 0x106d88684 in NetUpdate d_loop.c:242
    #9 0x106d89b36 in TryRunTics d_loop.c:741
    #10 0x106de158e in D_DoomLoop d_main.c:469
    #11 0x106de5678 in D_DoomMain d_main.c:1895
    #12 0x106d7f969 in main i_main.c:48
    #13 0x7fff5ff50144 in start (libdyld.dylib:x86_64+0x1144)

==24418==Register values:
rax = 0x000062600003c100  rbx = 0x000062600003c0fc  rcx = 0x0000000000000005  rdx = 0x000063200001ba58  
rdi = 0x000063200001ba60  rsi = 0x0000000000000645  rbp = 0x00007ffee8e81190  rsp = 0x00007ffee8e810a0  
 r8 = 0xfffffffffffffff8   r9 = 0x0000000000000002  r10 = 0x0000632000030000  r11 = 0x000000000000063f  
r12 = 0x0000000000000000  r13 = 0x0000000000000000  r14 = 0x00000000000015fd  r15 = 0x0000000000000008  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (libSDL2-2.0.0.dylib:x86_64+0xbd37) in SDL_ResampleAudio
==24418==ABORTING
Abort trap: 6

[devnexen]

My 2 cents about this topic, the above comment seems to confirm what I thought at this time
bradharding/doomretro#410 which originated that ticket (the solution for chocolate-doom is to adapt of course since those two client engines differ code-wise or looking at recent code change in the other client since it seemingly work fine now (e.g. bradharding/doomretro@70ba911#diff-c38e1305b41fcfdb9d2b7a3ca18569f9)). Is it a real SDL bug ? I do not know but if it is not and it really needs a bigger buffer than before, there is a need for explicitly mention it in the CHANGELOG, header code comment ... whatever ... from SDL side, because it is not really obvious.

[ndrchvzz]

The problem is the SDL_BuildAudioCVT() call in i_sdlsound.c.
When called with these arguments:

SDL_BuildAudioCVT(&convertor, AUDIO_U8, 1, 11025, AUDIO_S16LSB, 2, 44100));

one would expect convertor.len_mult to be set to 16 (2*2*4), but instead it's set 64.
The API of SDL_ConvertAudio() states that the size of the output buffer should be convertor.len * convertor.len_mult.
Our code however only allocates 16 * len, as it would be reasonable to do.
So this seems to be a bug in chocolate-doom, rather than in SDL2. We can't just try to guess the size of the output buffer by looking at the conversion parameters. So if for example we are converting to a sampling rate that is double the original, we can't just assume that the output buffer size should be double the input buffer size. It could be 4x, 8x, or whatever.
The best fix would be to use the returned convertor.len_mult and simply allocate the memory that it requires.
It doesn't seem so easy though due to the way the code is structured.
If you just want to play the game this is a quick and dirty fix for the time being:

--- a/src/i_sdlsound.c
+++ b/src/i_sdlsound.c
@@ -208,7 +208,7 @@ static allocated_sound_t *AllocateSound(sfxinfo_t *sfxinfo, size_t len)
     do
     {
-        snd = malloc(sizeof(allocated_sound_t) + len);
+        snd = malloc(4 * (sizeof(allocated_sound_t) + len));

         // Out of memory?  Try to free an old sound, then loop round
         // and try again.

[ericwa]

Here's an alternate patch:

diff --git a/src/i_sdlsound.c b/src/i_sdlsound.c
index 32653099..174fc376 100644
--- a/src/i_sdlsound.c
+++ b/src/i_sdlsound.c
@@ -631,11 +631,18 @@ static boolean ExpandSoundData_SDL(sfxinfo_t *sfxinfo,
                           AUDIO_U8, 1, samplerate,
                           mixer_format, mixer_channels, mixer_freq))
     {
-        convertor.buf = chunk->abuf;
+        int buf_len;
+
         convertor.len = length;
+        buf_len = convertor.len * convertor.len_mult;
+        convertor.buf = malloc(buf_len);
+        assert(convertor.buf != NULL);
         memcpy(convertor.buf, data, length);
 
         SDL_ConvertAudio(&convertor);
+
+        memcpy(chunk->abuf, convertor.buf, chunk->alen);
+        free(convertor.buf);
     }
     else
     {

The reason SDL_ConvertAudio requires a larger buffer in 2.0.6/2.0.7 is SDL2 got a "real" (FIR-filter) resampler (previous versions were very poor quality nearest/linear resamplers). Internally it doesn't resample in place now, but writes the output after the input part of the provided buffer, then copies it back to the start.

[ndrchvzz]

@ericwa I would have avoided creating the buf_len variable and just have the multiplication inlined.
Other than that I think this is great and gets the job done. I think we should do a pull request and get it merged.

[ericwa]

Sounds good - I'll make a PR.