Fix excessive stack usage when calling vorbis_analysis_wrote with lots of samples
#104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 25, 2023
AlexTMjugador
force-pushed
the
fix/unbounded-alloca
branch
from
352b150 to
c9694a0
Compare
…ots of samples `vorbis_analysis_wrote` increments `v->pcm_current` by `vals`, and this incremented value can be used by `_preextrapolate_helper` right after to allocate a float array in the stack `v->pcm_current` positions large. Clearly, since `alloca` does not check that there is enough stack space available to satisfy the allocation request, this can lead to a stack overflow and memory corruption, which at best have no effect, more likely cause segmentation faults, and at worst introduce security risks. The documentation for `vorbis_analysis_buffer` and `vorbis_analysis_wrote` does not specify a maximum value for `vals`. It states that "1024 is a reasonable choice", but callers are free to use larger or smaller counts as they wish. Therefore, `libvorbis` not handling this case is undesirable behavior. To better handle this case without throwing the performance benefits of `alloca` out the window, let's check whether the allocation would exceed 256 KiB (an estimate for the minimum stack space available is 1 MiB, which is [the default on Windows platforms](https://learn.microsoft.com/en-us/windows/win32/procthread/thread-stack-size)), and if so fall back to a heap allocated array. The heap array that may be allocated for this purpose is freed when `vorbis_dsp_clear` is called. `_preextrapolate_helper` takes neglible execution time compared to the encoding process for usual sample block sizes, though. Signed-off-by: Alejandro González <[email protected]>
AlexTMjugador
force-pushed
the
fix/unbounded-alloca
branch
from
c9694a0 to
f3c156f
Compare
tmatth
reviewed
Oct 6, 2025
tmatth
reviewed
Oct 6, 2025
|
Thanks for this contribution and for pinging us on IRC, just a few comments inline. Usually development happens on gitlab.xiph.org (which you'll need to have an account approved for, as you mentioned in #vorbis) but since this has been waiting a long time you can just apply your changes here and I'll ensure the MR gets created there. |
AlexTMjugador
commented
Oct 6, 2025
There was a problem hiding this comment.
Thanks a lot for the review and comments, @tmatth! 🎉
I've just pushed a commit that should address your review comments. Please let me know if there is anything else I can help with 🙌
|
Merged in https://gitlab.xiph.org/xiph/vorbis/-/merge_requests/38 (with the limit lowered to 8kB as suggested by @tterribe). Thanks for the contribution. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
vorbis_analysis_wroteincrementsv->pcm_currentbyvals, and this incremented value can be used by_preextrapolate_helperright after to allocate a float array in the stackv->pcm_currentpositions large. Clearly, sinceallocadoes not check that there is enough stack space available to satisfy the allocation request, this can lead to a stack overflow and memory corruption, which at best have no effect, more likely cause segmentation faults, and at worst introduce security risks.The documentation for
vorbis_analysis_bufferandvorbis_analysis_wrotedoes not specify a maximum value forvals. It states that "1024 is a reasonable choice", but callers are free to use larger or smaller counts as they wish. Therefore,libvorbisnot handling this case is undesirable behavior.To better handle this case without throwing the performance benefits of
allocaout the window, let's check whether the allocation would exceed 256 KiB (an estimate for the minimum stack space available is 1 MiB, which is the default on Windows platforms), and if so fall back to a heap allocated array. The heap array that may be allocated for this purpose is freed whenvorbis_dsp_clearis called._preextrapolate_helpertakes neglible execution time compared to the encoding process for usual sample block sizes, though.