fix(decoder): heap-allocate OggVorbis_File to prevent UAF on decoder move

Issue #35 reported a use-after-free bug after decoding a sample block
with the high-level `VorbisDecoder` struct, and then moving it, due to a
faulty assumption on our bindings that the internal `OggVorbis_File`
struct was safe to move alongside the `VorbisDecoder`.

I've independently verified that the issue is real and that the root cause
analysis outlined in #35 by the issue reporter is correct. Given such
root cause, `Box`ing the underlying `OggVorbis_File` is the only proper
fix for the problem, short of rewriting `libvorbis` internals.

As a part of the fix and investigation for this issue, I came up with a
regression test for it.

Fixes #35.

Author: AlexTMjugador

CHANGELOG.md

@@ -17,6 +17,11 @@ and this project adheres to
 
 - Documentation on how to use `vorbis_rs` with WebAssembly targets was added.
 
+### Fixed
+
+- Resolved a use-after-free memory safety issue caused by moving a
+`VorbisDecoder` after decoding a block of samples. (#35, thanks @EriKWDev)
+
 ## [0.5.5] - 2024-12-13
 
 ### Changed

packages/vorbis_rs/src/decoder/decoder_impl.rs

@@ -19,7 +19,7 @@ use crate::{common::VorbisError, decoder::VorbisAudioSamples};
 /// A decoder that turns a perceptually-encoded, non-chained Ogg Vorbis stream into
 /// blocks of planar, single-precision float audio samples.
 pub struct VorbisDecoder<R: Read> {
-	ogg_vorbis_file: OggVorbis_File,
+	ogg_vorbis_file: Box<OggVorbis_File>,
 	source: PhantomData<R>,
 	last_audio_block: Option<VorbisAudioSamples>
 }
@@ -33,14 +33,17 @@ impl<R: Read> VorbisDecoder<R> {
 	/// I/O errors that might happen during that operation will be returned to the
 	/// caller.
 	pub fn new<S: Into<Box<R>>>(source: S) -> Result<Self, VorbisError> {
-		let mut ogg_vorbis_file = MaybeUninit::uninit();
-
 		// The source read needs to be allocated in the heap (i.e., boxed) to have a
 		// constant memory address. Then leak it to a raw pointer to hand its ownership
 		// over to C code. Related, interesting read about trait objects and FFI:
 		// https://adventures.michaelfbryan.com/posts/ffi-safe-polymorphism-in-rust/
 		let source = Box::into_raw(source.into());
 
+		// The underlying `OggVorbis_File` struct also needs to have a constant memory address because
+		// it stores the vorbis_dsp_state and vorbis_block states by value, and the latter stores a
+		// pointer to the former that would be invalidated on move
+		let mut ogg_vorbis_file = Box::new_uninit();
+
 		// SAFETY: we assume ov_open_callbacks follows its documented contract
 		unsafe {
 			match vorbisfile_return_value_to_result!(ov_open_callbacks(
@@ -120,7 +123,7 @@ impl<R: Read> VorbisDecoder<R> {
 		// VorbisAudioSamples implementation for more safety information
 		unsafe {
 			let samples_read = vorbisfile_return_value_to_result!(ov_read_float(
-				&mut self.ogg_vorbis_file,
+				&mut *self.ogg_vorbis_file,
 				sample_buf.as_mut_ptr(),
 				2048, // Most stereo Ogg Vorbis files in the wild use a maximum block size of 2048 samples
 				current_bitstream.as_mut_ptr()
@@ -164,7 +167,7 @@ impl<R: Read> VorbisDecoder<R> {
 
 impl<R: Read> Drop for VorbisDecoder<R> {
 	fn drop(&mut self) {
-		unsafe { ov_clear(&mut self.ogg_vorbis_file) };
+		unsafe { ov_clear(&mut *self.ogg_vorbis_file) };
 	}
 }
 

packages/vorbis_rs/tests/regression_tests.rs

@@ -1,6 +1,9 @@
 //! Tests for ensuring past issues are kept fixed.
-
-use std::num::{NonZeroU8, NonZeroU32};
+use std::{
+	mem::{self, MaybeUninit},
+	num::{NonZeroU8, NonZeroU32},
+	slice
+};
 
 use vorbis_rs::{VorbisDecoder, VorbisEncoderBuilder};
 
@@ -56,3 +59,56 @@ fn issue_17() {
 		"The number of encoded samples must match the number of decoded samples"
 	);
 }
+
+#[doc = concat!(env!("CARGO_PKG_REPOSITORY"), "/issues/35")]
+#[test]
+fn issue_35() {
+	// To reliably showcase this issue about use-after-free due to a move, we need to store the
+	// decoder at a memory address where we can loudly invalidate any reads made to the address it
+	// was at before the move. A stack-allocated variable is inconvenient for this because the stack
+	// frames of any called function stay valid while they are part of the call stack, and the
+	// specifics of how their storage gets reused for new stack frames or variables is
+	// platform-specific. Therefore, we use a heap allocation through a `Vec` (see comments below
+	// for why this instead of, e.g., a `Box`)
+	let mut heap_decoder_buf = vec![
+		VorbisDecoder::<&[u8]>::new(
+			&include_bytes!(
+				"../../aotuv_lancer_vorbis_sys/src/8khz_500ms_mono_400hz_sine_wave.ogg"
+			)[..]
+		)
+		.unwrap(),
+	];
+
+	// Ensure all decoder state is initialized, which includes pointers in C structs
+	heap_decoder_buf[0].decode_audio_block().unwrap();
+
+	// Now that we have a fully initialized decoder on a heap-backed `Vec`, move it out of such `Vec`.
+	// Per Rust's move semantics, this effectively `memcpy`s the decoder to a memory address
+	// somewhere on the stack, rendering the previous heap address it was located at free for use by
+	// any other `Vec` elements, or released back to the OS if the backing buffer is freed or
+	// reallocated. Because we don't do the later by this point, the `Vec` buffer has capacity
+	// allocated and likely storing a copy of the single element we had in it (this would not
+	// necessarily be the case with a `Box`), so we can alter the memory representation of that copy
+	// through spare `Vec` capacity accessors (the `unsafe` block that follows is a backport to
+	// stable Rust of nightly Rust's MaybeUninit::as_bytes_mut`) to garbage bytes. In turn, the
+	// garbage bytes chosen practically guarantee that libvorbis attempts to reference invalid
+	// memory when any of its internal structs still refer to the freed, stale copy of the struct.
+	// TODO: replace the `unsafe` below with `MaybeUninit::as_bytes_mut` once it gets stabilized
+	let mut stack_decoder = heap_decoder_buf.remove(0);
+	for byte in unsafe {
+		slice::from_raw_parts_mut(
+			heap_decoder_buf.spare_capacity_mut()[0]
+				.as_mut_ptr()
+				.cast::<MaybeUninit<u8>>(),
+			mem::size_of::<VorbisDecoder<&[u8]>>()
+		)
+	} {
+		byte.write(0xCA); // Neither 0 nor 0xFF to avoid interpretation as sentinel values
+	}
+
+	// After the setup above, when affected by the use-after-free due to a move bug this will reliably
+	// cause an invalid memory address to be accessed, aborting the process with SIGSEGV on
+	// Unix-like systems or he STATUS_ACCESS_VIOLATION structured exception on Windows, and fail the
+	// test
+	stack_decoder.decode_audio_block().ok();
+}