Skip to content

Commit

Permalink
fix(enc): properly handle large buffers with libvorbis patch
Browse files Browse the repository at this point in the history
Issue #17 reported a crash I could reproduce when trying to encode very
large sample buffers. After some investigation, I narrowed down the
cause to an `alloca` whose allocation size was dependent on the input
buffer size, which incurs in undefined behavior if the stack is too
small. (This is why modern C coding guidelines discourage the usage of
`alloca`.)

I considered two approaches to fix this:

- The `vorbis_rs` crate could introduce a dependency on the `stacker`
  crate to allocate the stack on the heap if it turns out to be too
  small to hold this `alloca` buffer. However, that couples the
  `vorbis_rs` crate with an internal implementation detail of
  `libvorbis`, requires complex stack pointer manipulation techniques
  not available on all platforms, and introduces quite a few
  transitive dependencies.
- I could modify `libvorbis` to allocate this buffer on the heap if it
  is too big. This approach is very simple and, while it may come at a
  very minor performance cost for (re)allocating an array at most each
  time a block is submitted, it's otherwise similar to growing the stack
  on the heap.

I liked the second option more, so I went for it.

Fixes #17.
  • Loading branch information
AlexTMjugador committed Nov 25, 2023
1 parent 15eb84a commit 9ce5107
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 202 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ and this project adheres to

## [Unreleased]

### Fixed

- `VorbisEncoder::encode_audio_block` no longer causes a stack overflow in
practical scenarios when encoding a large sample buffer. TThis overflow
occurred when the available stack space was too small to handle the buffer,
and usually caused segmentation faults.
([#17](https://github.com/ComunidadAylas/vorbis-rs/issues/17), thanks @emoon)

### Changed

- Corrected some minor pedantic Clippy lints.
Expand Down
Loading

0 comments on commit 9ce5107

Please sign in to comment.