From 628d849ea73b794ed57f3096075dd00c34513c1a Mon Sep 17 00:00:00 2001 From: Mike Waychison Date: Wed, 21 Apr 2021 13:29:15 -0700 Subject: [PATCH 1/5] Bounce through SDL2 heap in AudioCVT::convert This commit rewrites AudioCVT::convert to bounce the audio buffer into a an SDL2 heap allocation, rather than trying to reuse the rust heap buffer. This is critical, as the underlying library warns internally that the buffer may be reallocated, breaking configurations where the library is not using the same heap as rust. The underlying implementation also notes that the underlying buffer may be transparently resized to larger than the output as part of the transformations, so relying on the buffer being capacity() bytes long to ensure the buffer is not re-allocated is a broken assumption. Closes #1096 --- src/sdl2/audio.rs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 1d7df568733..9c498f6fe80 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -1012,15 +1012,21 @@ impl AudioCVT { if self.raw.needed != 0 { let mut raw = self.raw; - // calculate the size of the dst buffer + // calculate the size of the dst buffer. use std::convert::TryInto; raw.len = src.len().try_into().expect("Buffer length overflow"); + + // This is more a suggestion, and not really a guarantee... let dst_size = self.capacity(src.len()); - let needed = dst_size - src.len(); - src.reserve_exact(needed); - // perform the conversion in place - raw.buf = src.as_mut_ptr(); + // Bounce into SDL2 heap allocation as SDL_ConvertAudio may rewrite the pointer. + raw.buf = sys::SDL_malloc(dst_size as u32) as *mut _; + if raw.buf.is_null() { + panic!("Failed SDL_malloc needed for SDL_ConvertAudio"); + } + // raw.buf is dst_size long, but we want to copy into only the first src.len bytes. + std::slice::from_raw_parts_mut(raw.buf, src.len()).copy_from_slice(src.as_ref()); + let ret = sys::SDL_ConvertAudio(&mut raw); // There's no reason for SDL_ConvertAudio to fail. // The only time it can fail is if buf is NULL, which it never is. @@ -1028,11 +1034,13 @@ impl AudioCVT { panic!("{}", get_error()) } - // return original buffer back to caller - debug_assert!(raw.len_cvt > 0); - debug_assert!(raw.len_cvt as usize <= src.capacity()); + // Bounce back into src, trying to re-use the same buffer. + let outlen: usize = raw.len_cvt.try_into().expect("Buffer size rollover"); + debug_assert!(outlen <= dst_size); + src.resize(outlen, 0); + src.copy_from_slice(std::slice::from_raw_parts_mut(raw.buf, outlen)); + sys::SDL_free(raw.buf as *mut _); - src.set_len(raw.len_cvt as usize); src } else { // The buffer remains unmodified From 02fdf0ddd08e6b4d8141539c030f52d887dcca5e Mon Sep 17 00:00:00 2001 From: Mike Waychison Date: Wed, 21 Apr 2021 14:09:12 -0700 Subject: [PATCH 2/5] Let the SDL_malloc cast work on 64bit Linux --- src/sdl2/audio.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 9c498f6fe80..5a6355d5e92 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -1020,7 +1020,7 @@ impl AudioCVT { let dst_size = self.capacity(src.len()); // Bounce into SDL2 heap allocation as SDL_ConvertAudio may rewrite the pointer. - raw.buf = sys::SDL_malloc(dst_size as u32) as *mut _; + raw.buf = sys::SDL_malloc(dst_size as _) as *mut _; if raw.buf.is_null() { panic!("Failed SDL_malloc needed for SDL_ConvertAudio"); } From 6e8f690a4fde8852aad6bba57289f3ad7fd821ae Mon Sep 17 00:00:00 2001 From: Mike Waychison Date: Wed, 21 Apr 2021 14:21:35 -0700 Subject: [PATCH 3/5] Reorder and add an assert --- src/sdl2/audio.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 5a6355d5e92..204515b158a 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -1012,19 +1012,19 @@ impl AudioCVT { if self.raw.needed != 0 { let mut raw = self.raw; - // calculate the size of the dst buffer. - use std::convert::TryInto; - raw.len = src.len().try_into().expect("Buffer length overflow"); - + // Calculate the size of the buffer we're handing to to SDL. // This is more a suggestion, and not really a guarantee... let dst_size = self.capacity(src.len()); // Bounce into SDL2 heap allocation as SDL_ConvertAudio may rewrite the pointer. raw.buf = sys::SDL_malloc(dst_size as _) as *mut _; + use std::convert::TryInto; + raw.len = src.len().try_into().expect("Buffer length overflow"); if raw.buf.is_null() { panic!("Failed SDL_malloc needed for SDL_ConvertAudio"); } // raw.buf is dst_size long, but we want to copy into only the first src.len bytes. + assert!(src.len() < dst_size); std::slice::from_raw_parts_mut(raw.buf, src.len()).copy_from_slice(src.as_ref()); let ret = sys::SDL_ConvertAudio(&mut raw); From 9691249c34ab492725c93a122387c3994f3ea62a Mon Sep 17 00:00:00 2001 From: Mike Waychison Date: Wed, 21 Apr 2021 14:23:10 -0700 Subject: [PATCH 4/5] Fix inequality in assertion --- src/sdl2/audio.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 204515b158a..0cf57bf774c 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -1024,7 +1024,7 @@ impl AudioCVT { panic!("Failed SDL_malloc needed for SDL_ConvertAudio"); } // raw.buf is dst_size long, but we want to copy into only the first src.len bytes. - assert!(src.len() < dst_size); + assert!(src.len() <= dst_size); std::slice::from_raw_parts_mut(raw.buf, src.len()).copy_from_slice(src.as_ref()); let ret = sys::SDL_ConvertAudio(&mut raw); From b67abb81af6a1a991a26a1b40e47bbec114b691c Mon Sep 17 00:00:00 2001 From: Mike Waychison Date: Wed, 21 Apr 2021 14:28:54 -0700 Subject: [PATCH 5/5] Style cleanup --- src/sdl2/audio.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/sdl2/audio.rs b/src/sdl2/audio.rs index 0cf57bf774c..754ec027b9c 100644 --- a/src/sdl2/audio.rs +++ b/src/sdl2/audio.rs @@ -1010,22 +1010,24 @@ impl AudioCVT { //! Certain conversions may cause buffer overflows. See AngryLawyer/rust-sdl2 issue #270. unsafe { if self.raw.needed != 0 { + use std::convert::TryInto; + use std::slice::from_raw_parts_mut; + let mut raw = self.raw; - // Calculate the size of the buffer we're handing to to SDL. + // Calculate the size of the buffer we're handing to SDL. // This is more a suggestion, and not really a guarantee... let dst_size = self.capacity(src.len()); // Bounce into SDL2 heap allocation as SDL_ConvertAudio may rewrite the pointer. - raw.buf = sys::SDL_malloc(dst_size as _) as *mut _; - use std::convert::TryInto; raw.len = src.len().try_into().expect("Buffer length overflow"); + raw.buf = sys::SDL_malloc(dst_size as _) as *mut _; if raw.buf.is_null() { panic!("Failed SDL_malloc needed for SDL_ConvertAudio"); } // raw.buf is dst_size long, but we want to copy into only the first src.len bytes. assert!(src.len() <= dst_size); - std::slice::from_raw_parts_mut(raw.buf, src.len()).copy_from_slice(src.as_ref()); + from_raw_parts_mut(raw.buf, src.len()).copy_from_slice(src.as_ref()); let ret = sys::SDL_ConvertAudio(&mut raw); // There's no reason for SDL_ConvertAudio to fail. @@ -1038,7 +1040,7 @@ impl AudioCVT { let outlen: usize = raw.len_cvt.try_into().expect("Buffer size rollover"); debug_assert!(outlen <= dst_size); src.resize(outlen, 0); - src.copy_from_slice(std::slice::from_raw_parts_mut(raw.buf, outlen)); + src.copy_from_slice(from_raw_parts_mut(raw.buf, outlen)); sys::SDL_free(raw.buf as *mut _); src