From dddf65dba5c9bef0e5deabcb13f1fac4ea802621 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Sun, 18 Jan 2026 15:37:37 +0800 Subject: [PATCH] Fix audio race condition with heap allocation This replaces stack-local buffers with heap allocation for audio threads to eliminate use-after-free when threads outlive the calling function. - play_sfx/play_music: heap-allocate sound_t + data buffer together - sfx_handler/music_handler: take ownership and free argument - Use detached thread for native (non-blocking), joinable for EMSCRIPTEN - Handle pthread_create failure by freeing allocation - music_handler: free previous mid/music_midi_data to prevent leak - shutdown_audio: stop playback first, proper resource cleanup - Fix EMSCRIPTEN double-join by not setting init flags after immediate join - Validate size bounds before allocation to prevent DoS from guest --- src/syscall_sdl.c | 207 +++++++++++++++++++++++++++++++--------------- 1 file changed, 142 insertions(+), 65 deletions(-) diff --git a/src/syscall_sdl.c b/src/syscall_sdl.c index c1cd8c4c..6c412403 100644 --- a/src/syscall_sdl.c +++ b/src/syscall_sdl.c @@ -94,15 +94,19 @@ typedef struct sound { } sound_t; /* SDL-mixer-related and music-related variables */ -static pthread_t music_thread; static uint8_t *music_midi_data; #if RV32_HAS(SDL_MIXER) static Mix_Music *mid; /* SDL-mixer-related and sfx-related variables */ -static pthread_t sfx_thread; static Mix_Chunk *sfx_chunk; #endif + +#ifdef __EMSCRIPTEN__ +/* Thread handles only needed for EMSCRIPTEN joinable threads */ +static pthread_t music_thread; +static pthread_t sfx_thread; +#endif static uint8_t *sfx_samples; static uint32_t nr_sfx_samples; static int chan; @@ -742,12 +746,16 @@ static void *sfx_handler(void *arg) memcpy(sfx_samples, ptr, sizeof(uint8_t) * nr_sfx_samples); sfx_chunk = Mix_QuickLoad_RAW(sfx_samples, nr_sfx_samples); - if (!sfx_chunk) + if (!sfx_chunk) { + free(sfx); return NULL; + } chan = Mix_PlayChannel(-1, sfx_chunk, 0); - if (chan == -1) + if (chan == -1) { + free(sfx); return NULL; + } if (*ptr & 0x3) { /* Doom, multiplied by 8 because sfx->volume's max is 15 */ @@ -759,6 +767,7 @@ static void *sfx_handler(void *arg) Mix_Volume(chan, (sfx->volume + 1) % 128); } + free(sfx); return NULL; } @@ -767,23 +776,35 @@ static void *music_handler(void *arg) sound_t *music = (sound_t *) arg; int looping = music->looping ? -1 : 1; + /* Free previous MIDI data */ free(music_midi_data); + music_midi_data = NULL; + + /* Free previous music to prevent memory leak */ + if (mid) { + Mix_HaltMusic(); + Mix_FreeMusic(mid); + mid = NULL; + } music_midi_data = mus2midi(music->data, (int *) &music->size); if (!music_midi_data) { rv_log_error("mus2midi() failed"); + free(music); return NULL; } SDL_RWops *rwops = SDL_RWFromMem(music_midi_data, music->size); if (!rwops) { rv_log_error("SDL_RWFromMem failed: %s", SDL_GetError()); + free(music); return NULL; } mid = Mix_LoadMUSType_RW(rwops, MUS_MID, SDL_TRUE); if (!mid) { rv_log_error("Mix_LoadMUSType_RW failed: %s", Mix_GetError()); + free(music); return NULL; } @@ -794,9 +815,11 @@ static void *music_handler(void *arg) if (Mix_PlayMusic(mid, looping) == -1) { rv_log_error("Mix_PlayMusic failed: %s", Mix_GetError()); + free(music); return NULL; } + free(music); return NULL; } @@ -808,21 +831,15 @@ static void play_sfx(riscv_t *rv) int volume = rv_get_reg(rv, rv_reg_a2); sfxinfo_t sfxinfo; - uint8_t sfx_data[SFX_SAMPLE_SIZE]; + uint32_t sfx_data_size; + #if RV32_HAS(SYSTEM_MMIO) uint32_t addr = rv->io.mem_translate(rv, sfxinfo_addr, R); memory_read(attr->mem, (uint8_t *) &sfxinfo, addr, sizeof(sfxinfo_t)); uint32_t sfx_data_offset = *((uint32_t *) ((uint8_t *) attr->mem->mem_base + addr)); - uint32_t sfx_data_size = - *(uint32_t *) ((uint8_t *) attr->mem->mem_base + addr + 4); - uint32_t sfx_data_vaddr = sfx_data_offset; - uint8_t *sfx_data_ptr = &sfx_data[0]; - remain_size = sfx_data_size; - curr_offset = 0; - - GET_SFX_DATA_FROM_RANDOM_PAGE(sfx_data_vaddr, sfx_data_ptr); + sfx_data_size = *(uint32_t *) ((uint8_t *) attr->mem->mem_base + addr + 4); #else memory_read(attr->mem, (uint8_t *) &sfxinfo, sfxinfo_addr, sizeof(sfxinfo_t)); @@ -832,28 +849,57 @@ static void play_sfx(riscv_t *rv) * various applications when accessing different sfxinfo_t instances. */ uint32_t sfx_data_offset = *((uint32_t *) &sfxinfo); - uint32_t sfx_data_size = *(uint32_t *) ((uint32_t *) &sfxinfo + 1); - memory_read(attr->mem, sfx_data, sfx_data_offset, - sizeof(uint8_t) * sfx_data_size); + sfx_data_size = *(uint32_t *) ((uint32_t *) &sfxinfo + 1); #endif - sound_t sfx = { - .data = sfx_data, - .size = sfx_data_size, - .volume = volume, - }; #if RV32_HAS(SDL_MIXER) - pthread_create(&sfx_thread, NULL, sfx_handler, &sfx); - sfx_thread_init = true; + /* Validate size to prevent excessive allocation from untrusted guest */ + if (sfx_data_size == 0 || sfx_data_size > SFX_SAMPLE_SIZE) + return; + + /* Heap allocate sound_t + data buffer together (thread takes ownership) */ + sound_t *sfx = malloc(sizeof(sound_t) + sfx_data_size); + if (!sfx) + return; + + sfx->data = (uint8_t *) (sfx + 1); /* Data follows the struct */ + sfx->size = sfx_data_size; + sfx->volume = volume; + +#if RV32_HAS(SYSTEM_MMIO) + uint32_t sfx_data_vaddr = sfx_data_offset; + uint8_t *sfx_data_ptr = sfx->data; + remain_size = sfx_data_size; + curr_offset = 0; + + GET_SFX_DATA_FROM_RANDOM_PAGE(sfx_data_vaddr, sfx_data_ptr); +#else + memory_read(attr->mem, sfx->data, sfx_data_offset, + sizeof(uint8_t) * sfx_data_size); #endif - /* FIXME: In web browser runtime, web workers in thread pool do not reap - * after sfx_handler return, thus we have to join them. sfx_handler does not - * contain infinite loop,so do not worry to be stalled by it */ + #ifdef __EMSCRIPTEN__ -#if RV32_HAS(SDL_MIXER) + /* Web browser: use joinable thread and wait for completion */ + if (pthread_create(&sfx_thread, NULL, sfx_handler, sfx) != 0) { + free(sfx); + return; + } pthread_join(sfx_thread, NULL); + /* Thread already joined - don't join again in shutdown_audio */ +#else + /* Native: use detached thread for non-blocking playback */ + pthread_t thread; + pthread_attr_t thread_attr; + pthread_attr_init(&thread_attr); + pthread_attr_setdetachstate(&thread_attr, PTHREAD_CREATE_DETACHED); + if (pthread_create(&thread, &thread_attr, sfx_handler, sfx) != 0) { + pthread_attr_destroy(&thread_attr); + free(sfx); + return; + } + pthread_attr_destroy(&thread_attr); #endif -#endif +#endif /* RV32_HAS(SDL_MIXER) */ } static void play_music(riscv_t *rv) @@ -865,7 +911,8 @@ static void play_music(riscv_t *rv) int looping = rv_get_reg(rv, rv_reg_a3); musicinfo_t musicinfo; - uint8_t music_data[MUSIC_MAX_SIZE]; + uint32_t music_data_size; + #if RV32_HAS(SYSTEM_MMIO) uint32_t addr = rv->io.mem_translate(rv, musicinfo_addr, R); memory_read(attr->mem, (uint8_t *) &musicinfo, addr, sizeof(musicinfo_t)); @@ -876,14 +923,8 @@ static void play_music(riscv_t *rv) */ uint32_t music_data_offset = *((uint32_t *) ((uint8_t *) attr->mem->mem_base + addr)); - uint32_t music_data_size = + music_data_size = *(uint32_t *) ((uint8_t *) attr->mem->mem_base + addr + 4); - uint32_t music_data_vaddr = music_data_offset; - uint8_t *music_data_ptr = &music_data[0]; - remain_size = music_data_size; - curr_offset = 0; - - GET_MUSIC_DATA_FROM_RANDOM_PAGE(music_data_vaddr, music_data_ptr); #else memory_read(attr->mem, (uint8_t *) &musicinfo, musicinfo_addr, sizeof(musicinfo_t)); @@ -893,28 +934,57 @@ static void play_music(riscv_t *rv) * various applications when accessing different sfxinfo_t instances. */ uint32_t music_data_offset = *((uint32_t *) &musicinfo); - uint32_t music_data_size = *(uint32_t *) ((uint32_t *) &musicinfo + 1); - memory_read(attr->mem, music_data, music_data_offset, music_data_size); + music_data_size = *(uint32_t *) ((uint32_t *) &musicinfo + 1); #endif - sound_t music = { - .data = music_data, - .size = music_data_size, - .looping = looping, - .volume = volume, - }; #if RV32_HAS(SDL_MIXER) - pthread_create(&music_thread, NULL, music_handler, &music); - music_thread_init = true; + /* Validate size to prevent excessive allocation from untrusted guest */ + if (music_data_size == 0 || music_data_size > MUSIC_MAX_SIZE) + return; + + /* Heap allocate sound_t + data buffer together (thread takes ownership) */ + sound_t *music = malloc(sizeof(sound_t) + music_data_size); + if (!music) + return; + + music->data = (uint8_t *) (music + 1); /* Data follows the struct */ + music->size = music_data_size; + music->looping = looping; + music->volume = volume; + +#if RV32_HAS(SYSTEM_MMIO) + uint32_t music_data_vaddr = music_data_offset; + uint8_t *music_data_ptr = music->data; + remain_size = music_data_size; + curr_offset = 0; + + GET_MUSIC_DATA_FROM_RANDOM_PAGE(music_data_vaddr, music_data_ptr); +#else + memory_read(attr->mem, music->data, music_data_offset, music_data_size); #endif - /* FIXME: In web browser runtime, web workers in thread pool do not reap - * after music_handler return, thus we have to join them. music_handler does - * not contain infinite loop,so do not worry to be stalled by it */ + #ifdef __EMSCRIPTEN__ -#if RV32_HAS(SDL_MIXER) + /* Web browser: use joinable thread and wait for completion */ + if (pthread_create(&music_thread, NULL, music_handler, music) != 0) { + free(music); + return; + } pthread_join(music_thread, NULL); + /* Thread already joined - don't join again in shutdown_audio */ +#else + /* Native: use detached thread for non-blocking playback */ + pthread_t thread; + pthread_attr_t thread_attr; + pthread_attr_init(&thread_attr); + pthread_attr_setdetachstate(&thread_attr, PTHREAD_CREATE_DETACHED); + if (pthread_create(&thread, &thread_attr, music_handler, music) != 0) { + pthread_attr_destroy(&thread_attr); + free(music); + return; + } + pthread_attr_destroy(&thread_attr); #endif -#endif +#endif /* RV32_HAS(SDL_MIXER) */ } static void stop_music() @@ -965,27 +1035,34 @@ static void init_audio(void) static void shutdown_audio() { +#if RV32_HAS(SDL_MIXER) + /* Stop all playback first */ + Mix_HaltMusic(); + Mix_HaltChannel(-1); + +#ifdef __EMSCRIPTEN__ /* - * sfx_thread and music_thread might contain invalid identifiers. - * Additionally, there is no built-in method to verify the validity - * of a pthread_t type. Therefore, the sfx_thread_init and music_thread_init - * flags are used for validation, ensuring that pthread_join always operates - * on a valid pthread_t identifier. + * For EMSCRIPTEN, threads are joinable. Join them if initialized to ensure + * handlers have completed before freeing resources. */ - -#if RV32_HAS(SDL_MIXER) - if (music_thread_init) { - stop_music(); + if (music_thread_init) pthread_join(music_thread, NULL); + if (sfx_thread_init) + pthread_join(sfx_thread, NULL); +#endif + + /* Free music resources */ + if (mid) { Mix_FreeMusic(mid); - free(music_midi_data); - music_midi_data = NULL; + mid = NULL; } + free(music_midi_data); + music_midi_data = NULL; - if (sfx_thread_init) { - pthread_join(sfx_thread, NULL); - Mix_HaltChannel(-1); + /* Free sfx resources */ + if (sfx_chunk) { Mix_FreeChunk(sfx_chunk); + sfx_chunk = NULL; } Mix_CloseAudio();