Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stb_vorbis: fix slow loads and leaks in start_decoder. #1174

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AliceLR
Copy link

@AliceLR AliceLR commented Jul 16, 2021

This patch from libxmp fixes the following issues:

  • Error checks in start_decoder when loading the lengths could return without freeing the lengths array, causing leaks. OGG_leaks.zip
  • Missing EOF checks in start_decoder could contribute to very slow loads with corrupt/invalid files. This includes one check that originally checked for an EOP return value from get_bits—as far as I can tell, get_bits never actually returns this value. I don't remember exactly which of these missing checks contributed the most to slow loads, but IIRC I added most of them after reading values that get iterated on by loops. OGG_slow_loads.zip

@nothings
Copy link
Owner

nothings commented Jul 16, 2021

This looks reasonable, but I wonder if we should instead do some systematic thing for the setup_temp_frees where they're all tracked and vorbis_deinit() cleans them up if they didn't get deallocated explicitly. I.e. make 'setup temp free' explicitly zero the pointer it's handed, and then they're all explicitly stored off of f-> or c->. If you're using f->alloc.alloc_buffer we do nothing with temp mem during deinit(), which means deinit() doesn't have to worry about freeing the allocations in stack order.

@AliceLR
Copy link
Author

AliceLR commented Jul 16, 2021

I pushed an update that implements (roughly) what you described. Since the buffers are never allocated for more than one codebook at a time I put them all in f->.

@nothings
Copy link
Owner

Looks good to me with github diff, I'll need to review it more carefully in the full file.

@AliceLR
Copy link
Author

AliceLR commented Jul 16, 2021

Ran a brief libFuzzer+ASan run on this patch and it looks like there are still issues to work out—will update.

edit: specifically the issue is that storing the temporary codewords allocation to both c->codewords and f->temp_codewords predictably causes issues in vorbis_deinit.

@AliceLR AliceLR force-pushed the vorbis-fix-slow-load-and-leaks branch from 86a7457 to a5295ad Compare July 16, 2021 09:06
@nothings
Copy link
Owner

Do you have more tests you can run, or is that it?

@AliceLR
Copy link
Author

AliceLR commented Jul 16, 2021

I don't have any other tests that would be relevant to finding leaks/double frees, nor do I have a particularly good variety of invalid files to test against. No other issues related to this patch have showed up yet in my limited fuzzing.

@AliceLR
Copy link
Author

AliceLR commented Jul 16, 2021

My tester found some new slow load cases that might be due to another missed EOF check here, will follow up again: OGG_slow_loads2.zip

No further crashes related to this patch, though it looks like there may be an unrelated buffer overflow in codebook_decode_deinterleave_repeat.

@AliceLR
Copy link
Author

AliceLR commented Jul 16, 2021

It looks like the path the slow loads follow is valid: when ordered is true, the codebook can contain an absurd number of entries without needing to read most of them. There is an EOF check for this, it just doesn't read enough data to matter. (The slow portion of the load is calling qsort on 16m entries.)

@AliceLR AliceLR force-pushed the vorbis-fix-slow-load-and-leaks branch from a5295ad to 2ec4a48 Compare January 1, 2022 11:43
@AliceLR
Copy link
Author

AliceLR commented Jan 1, 2022

I've updated this patch to remove (void **)&pointer usage, which flags warnings in old GCC versions (cited version was 4.2) as well as newer versions with -fstrict-aliasing -Wstrict-aliasing=2 (tested with GCC 11.2). (Found and patched by @sezero.)

sezero added a commit to icculus/SDL_sound that referenced this pull request Jan 2, 2022
... and handle setup_temp_free for error cases in vorbis_deinit.

Patch by Alice Rowan.
Upstream: nothings/stb#1174
sezero added a commit to libsdl-org/SDL_mixer that referenced this pull request May 21, 2022
- Keeps the original stb_vorbis as intact as possible so
  that future updates from mainstream would be painless.
- Reduces STB_VORBIS_MAX_CHANNELS from 16 to 6 (objections?)
- Adds several missing libm function overrides,
- SDL_mixer now requires SDL >= 2.0.9 because of SDL_exp()
- Fixes slow loads and leaks in start_decoder:
  nothings/stb#1174
- Fixes submap array out-of-bounds indexing bug:
  nothings/stb#1312
- Replace signed overflow clamps with unsigned overflow:
  nothings/stb#1168
- Replaces alloca() usage with setup_malloc() (from libxmp.)
- Fixes '-Wmaybe-uninitialized' warnings in get_seek_page_info:
  nothings/stb#1172
- A minor UBSan fix and suppression:
  nothings/stb#1168
- Fixes signature of dummy realloc() for STB_VORBIS_NO_CRT:
  nothings/stb#1198
- Renames BUFFER_SIZE macro to STB_BUFFER_SIZE:
  nothings/stb#1078
- Pulls in sample-accurate offset patch of Vitaly Novichkov:
  (stb_vorbis_get_playback_sample_offset, because it's used
  in OGG_Tell and OGG_GetSome):
  nothings/stb#1294
  nothings/stb#1295
- Fixes a few warnings here and there in some environments.
- Replaces two dummy '(void) 0' with 'do {} while(0)'
sezero added a commit to libsdl-org/SDL_mixer that referenced this pull request May 21, 2022
- Keeps the original stb_vorbis as intact as possible so
  that future updates from mainstream would be painless.
- Reduces STB_VORBIS_MAX_CHANNELS from 16 to 6 (objections?)
- Adds several missing libm function overrides,
- SDL_mixer now requires SDL >= 2.0.9 because of SDL_exp()
- Fixes slow loads and leaks in start_decoder:
  nothings/stb#1174
- Fixes submap array out-of-bounds indexing bug:
  nothings/stb#1312
- Replace signed overflow clamps with unsigned overflow:
  nothings/stb#1168
- Replaces alloca() usage with setup_malloc() (from libxmp.)
- Fixes '-Wmaybe-uninitialized' warnings in get_seek_page_info:
  nothings/stb#1172
- A minor UBSan fix and suppression:
  nothings/stb#1168
- Fixes signature of dummy realloc() for STB_VORBIS_NO_CRT:
  nothings/stb#1198
- Renames BUFFER_SIZE macro to STB_BUFFER_SIZE:
  nothings/stb#1078
- Pulls in sample-accurate offset patch of Vitaly Novichkov:
  (stb_vorbis_get_playback_sample_offset, because it's used
  in OGG_Tell and OGG_GetSome):
  nothings/stb#1294
  nothings/stb#1295
- Fixes a few warnings here and there in some environments.
- Replaces two dummy '(void) 0' with 'do {} while(0)'
sezero pushed a commit to sezero/stb that referenced this pull request Dec 12, 2023
sezero pushed a commit to sezero/stb that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants