Undefined behaviour in vorbis_rs

[jasonyu1996]

Hi! In vorbis_rs, the encoder state is created with vorbis_info, vorbis_dsp_state, and vorbis_block all owned by the VorbisEncodingState object, but with pointers pointing to one another.

vorbis-rs/packages/vorbis_rs/src/encoder/encoder_util.rs

Lines 20 to 50 in c01a3ea

	pub fn new(mut vorbis_info: VorbisInfo) -> Result<Self, VorbisError> {
	// NOTE: stable-friendly version of Box::new_uninit
	let mut vorbis_dsp_state = Box::new(MaybeUninit::uninit());
	let mut vorbis_block = Box::new(MaybeUninit::uninit());
	// SAFETY: we assume vorbis_analysis_init and vorbis_block_init follow
	// their documented contract. The structs are wrapped in boxes to ensure
	// they live at constant addresses in memory, which is necessary because
	// internal libvorbis encoding state stores pointers to such addresses. We
	// take the ownership of vorbis_info even though we don't use it to ensure
	// its lifetime is as long as the lifetime of the encoding state and that
	// client code does not corrupt it
	unsafe {
	libvorbis_return_value_to_result!(vorbis_analysis_init(
	vorbis_dsp_state.as_mut_ptr(),
	&mut *vorbis_info.vorbis_info
	))?;
	let mut vorbis_dsp_state = assume_init_box(vorbis_dsp_state);
	libvorbis_return_value_to_result!(vorbis_block_init(
	&mut *vorbis_dsp_state,
	vorbis_block.as_mut_ptr()
	))?;
	Ok(Self {
	vorbis_info,
	vorbis_dsp_state,
	vorbis_block: assume_init_box(vorbis_block)
	})
	}
	}

https://github.com/ComunidadAylas/vorbis-aotuv-lancer/blob/1c49413115971752e360025af58f0e418106e7b9/lib/block.c#L84-L107

Then mutation is performed using such pointers rather than through those owner references or references/pointers derived from them:

https://github.com/ComunidadAylas/vorbis-aotuv-lancer/blob/1c49413115971752e360025af58f0e418106e7b9/lib/analysis.c#L40

This is an undefined behaviour and may cause miscompilation. A simplified demo https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=a98129c85a837c05db5958f33c24fe18 shows different results under debug and release. MIRI can catch this issue in the simplified demo but unfortunately does not work for the original code because it does not support FFI.

[AlexTMjugador]

Hello, thank you so much for reporting this issue!

I was not aware of the consequences of pointer aliasing in the context of Box ownership semantics, so your report is incredibly valuable: this is something I'd have likely overlooked otherwise. While researching the topic further, I found several useful resources for me and potentially other people under similar circumstances: the "Behavior considered undefined" page in the Rust reference, a really interesting blog post titled "Box Is a Unique Type", and a recent addition to the documentation for the std::boxed module. These resources led me to a helpful comment by @RalfJung:

I guess to be on the safe side I should use more unsafe and get away from Box as early as possible, so all the raw pointers are derived from the result of Box::into_raw, and not from when the box is live (even though they have the same address they have different provenance?).

Indeed, that is the correct approach. Mixing Box and raw ptr accesses to the same location is running a high risk of violating aliasing guarantees.

Therefore, I addressed the undefined behavior in question in this commit by modifying the code to bail out of Box's non-aliasing semantics as soon as possible. To avoid leaking memory with the resulting raw pointers, I manually reconstruct the Box only when it's time to free them, after calling the corresponding teardown C functions; the primary reason I had kept the Box around was to leverage its automatic memory cleanup on drop, after all.

To validate this approach, I adapted your reduced playground example using the same fix and successfully tested it with Miri.

I'd greatly appreciate if you could review the changes in the referenced commit to confirm whether the fix looks good to you as well, just to make sure I haven't missed any other thing 😇

[jasonyu1996]

Hello, thank you so much for reporting this issue!

I was not aware of the consequences of pointer aliasing in the context of Box ownership semantics, so your report is incredibly valuable: this is something I'd have likely overlooked otherwise. While researching the topic further, I found several useful resources for me and potentially other people under similar circumstances: the "Behavior considered undefined" page in the Rust reference, a really interesting blog post titled "Box Is a Unique Type", and a recent addition to the documentation for the std::boxed module. These resources led me to a helpful comment by @RalfJung:

I guess to be on the safe side I should use more unsafe and get away from Box as early as possible, so all the raw pointers are derived from the result of Box::into_raw, and not from when the box is live (even though they have the same address they have different provenance?).

Indeed, that is the correct approach. Mixing Box and raw ptr accesses to the same location is running a high risk of violating aliasing guarantees.

Therefore, I addressed the undefined behavior in question in this commit by modifying the code to bail out of Box's non-aliasing semantics as soon as possible. To avoid leaking memory with the resulting raw pointers, I manually reconstruct the Box only when it's time to free them, after calling the corresponding teardown C functions; the primary reason I had kept the Box around was to leverage its automatic memory cleanup on drop, after all.

To validate this approach, I adapted your reduced playground example using the same fix and successfully tested it with Miri.

I'd greatly appreciate if you could review the changes in the referenced commit to confirm whether the fix looks good to you as well, just to make sure I haven't missed any other thing 😇

Thanks! I will check out the commits when I get the time.