From 698da91b6dfd85331583d1a44f77af41260c2c43 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 24 Apr 2024 07:02:15 +0200 Subject: [PATCH 01/13] fix undefined behavior in `read_pixels` --- src/sdl2/render.rs | 56 ++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/src/sdl2/render.rs b/src/sdl2/render.rs index cb645752b9..b0f2164f7b 100644 --- a/src/sdl2/render.rs +++ b/src/sdl2/render.rs @@ -1446,37 +1446,35 @@ impl Canvas { rect: R, format: pixels::PixelFormatEnum, ) -> Result, String> { - unsafe { - let rect = rect.into(); - let (actual_rect, w, h) = match rect { - Some(ref rect) => (rect.raw(), rect.width() as usize, rect.height() as usize), - None => { - let (w, h) = self.output_size()?; - (ptr::null(), w as usize, h as usize) - } - }; + let rect = rect.into(); + let (actual_rect, w, h) = match rect { + Some(ref rect) => (rect.raw(), rect.width() as usize, rect.height() as usize), + None => { + let (w, h) = self.output_size()?; + (ptr::null(), w as usize, h as usize) + } + }; - let pitch = w * format.byte_size_per_pixel(); // calculated pitch - let size = format.byte_size_of_pixels(w * h); - let mut pixels = Vec::with_capacity(size); - pixels.set_len(size); - - // Pass the interior of `pixels: Vec` to SDL - let ret = { - sys::SDL_RenderReadPixels( - self.context.raw, - actual_rect, - format as u32, - pixels.as_mut_ptr() as *mut c_void, - pitch as c_int, - ) - }; + let pitch = w * format.byte_size_per_pixel(); // calculated pitch + let size = format.byte_size_of_pixels(w * h); + let mut pixels = Vec::with_capacity(size); - if ret == 0 { - Ok(pixels) - } else { - Err(get_error()) - } + // Pass the interior of `pixels: Vec` to SDL + let ret = unsafe { + sys::SDL_RenderReadPixels( + self.context.raw, + actual_rect, + format as u32, + pixels.as_mut_ptr() as *mut c_void, + pitch as c_int, + ) + }; + + if ret == 0 { + unsafe { pixels.set_len(size) }; + Ok(pixels) + } else { + Err(get_error()) } } From 152221268495c2929135c59608bbc92471f50fee Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 24 Apr 2024 07:18:29 +0200 Subject: [PATCH 02/13] fix undefined behavior in copy_ex --- sdl2-sys/sdl_bindings.rs | 2 +- src/sdl2/render.rs | 18 +++++++----------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/sdl2-sys/sdl_bindings.rs b/sdl2-sys/sdl_bindings.rs index b2bc6ca8ea..5d57536362 100644 --- a/sdl2-sys/sdl_bindings.rs +++ b/sdl2-sys/sdl_bindings.rs @@ -24387,7 +24387,7 @@ extern "C" { dstrect: *const SDL_Rect, angle: f64, center: *const SDL_Point, - flip: SDL_RendererFlip, + flip: u32, ) -> libc::c_int; } extern "C" { diff --git a/src/sdl2/render.rs b/src/sdl2/render.rs index b0f2164f7b..f857944553 100644 --- a/src/sdl2/render.rs +++ b/src/sdl2/render.rs @@ -1397,17 +1397,13 @@ impl Canvas { P: Into>, { use crate::sys::SDL_RendererFlip::*; - let flip = unsafe { - match (flip_horizontal, flip_vertical) { - (false, false) => SDL_FLIP_NONE, - (true, false) => SDL_FLIP_HORIZONTAL, - (false, true) => SDL_FLIP_VERTICAL, - (true, true) => transmute::( - transmute::(SDL_FLIP_HORIZONTAL) - | transmute::(SDL_FLIP_VERTICAL), - ), - } - }; + let mut flip = 0; + if flip_horizontal { + flip |= SDL_FLIP_HORIZONTAL as u32; + } + if flip_vertical { + flip |= SDL_FLIP_VERTICAL as u32; + } let ret = unsafe { sys::SDL_RenderCopyEx( From bcbf21d589c48cba7c013c43c3b1d07bdbd32364 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 24 Apr 2024 07:29:52 +0200 Subject: [PATCH 03/13] fix undefined behavior in Point::raw_slice and Rect::raw_slice Point was not layout compatible with sys::SDL_Point, making the transmute unsound. the same for Rect and sys::SDL_Rect. using repr(transparent) on the wrapper types fixes this. --- src/sdl2/rect.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sdl2/rect.rs b/src/sdl2/rect.rs index c0519e6fbc..5a1f074ba8 100644 --- a/src/sdl2/rect.rs +++ b/src/sdl2/rect.rs @@ -67,6 +67,7 @@ fn clamped_mul(a: i32, b: i32) -> i32 { /// recommended to use `Option`, with `None` representing an empty /// rectangle (see, for example, the output of the /// [`intersection`](#method.intersection) method). +#[repr(transparent)] #[derive(Clone, Copy)] pub struct Rect { raw: sys::SDL_Rect, @@ -728,6 +729,7 @@ impl BitOr for Rect { } /// Immutable point type, consisting of x and y. +#[repr(transparent)] #[derive(Copy, Clone)] pub struct Point { raw: sys::SDL_Point, From f374447d103898542afcc789e0f00d0667759ae2 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 24 Apr 2024 07:46:34 +0200 Subject: [PATCH 04/13] remove unnecessary transmute call --- src/sdl2/rwops.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sdl2/rwops.rs b/src/sdl2/rwops.rs index d48332b7e8..df8aa5ac39 100644 --- a/src/sdl2/rwops.rs +++ b/src/sdl2/rwops.rs @@ -4,7 +4,6 @@ use libc::{c_char, c_int, size_t}; use std::ffi::CString; use std::io; use std::marker::PhantomData; -use std::mem::transmute; use std::path::Path; use crate::sys; @@ -179,7 +178,7 @@ impl<'a> io::Seek for RWops<'a> { io::SeekFrom::End(pos) => (sys::RW_SEEK_END, pos), io::SeekFrom::Current(pos) => (sys::RW_SEEK_CUR, pos), }; - let ret = unsafe { ((*self.raw).seek.unwrap())(self.raw, offset, transmute(whence)) }; + let ret = unsafe { ((*self.raw).seek.unwrap())(self.raw, offset, whence as i32) }; if ret == -1 { Err(io::Error::last_os_error()) } else { From 2accf9909cc3342176084897d4819ef5b08d43b6 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 24 Apr 2024 09:37:45 +0200 Subject: [PATCH 05/13] dont leak log message string --- src/sdl2/log.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdl2/log.rs b/src/sdl2/log.rs index 2303d89a38..6e62d8de03 100644 --- a/src/sdl2/log.rs +++ b/src/sdl2/log.rs @@ -101,6 +101,6 @@ pub fn log(message: &str) { let message = message.replace('%', "%%"); let message = CString::new(message).unwrap(); unsafe { - crate::sys::SDL_Log(message.into_raw()); + crate::sys::SDL_Log(message.as_ptr()); } } From 3487ca329177a6022ee369149c7fc016719d8d11 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Wed, 24 Apr 2024 17:16:45 +0200 Subject: [PATCH 06/13] fix copy_ex in a different way that does not require changing sdl2-sys/sdl_bindings.rs --- sdl2-sys/sdl_bindings.rs | 2 +- src/sdl2/render.rs | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/sdl2-sys/sdl_bindings.rs b/sdl2-sys/sdl_bindings.rs index 5d57536362..b2bc6ca8ea 100644 --- a/sdl2-sys/sdl_bindings.rs +++ b/sdl2-sys/sdl_bindings.rs @@ -24387,7 +24387,7 @@ extern "C" { dstrect: *const SDL_Rect, angle: f64, center: *const SDL_Point, - flip: u32, + flip: SDL_RendererFlip, ) -> libc::c_int; } extern "C" { diff --git a/src/sdl2/render.rs b/src/sdl2/render.rs index f857944553..081eb20ab9 100644 --- a/src/sdl2/render.rs +++ b/src/sdl2/render.rs @@ -1396,6 +1396,22 @@ impl Canvas { R2: Into>, P: Into>, { + // This function doesn't use sys::SDL_RenderCopyEx because its signature does not allow + // SDL_FLIP_HORIZONTAL | SDL_FLIP_VERTICAL for parameter 'flip'. Here, a u32 is used + // instead of SDL_RendererFlip, which does allow that value. + + extern "C" { + fn SDL_RenderCopyEx( + renderer: *mut sys::SDL_Renderer, + texture: *mut sys::SDL_Texture, + srcrect: *const sys::SDL_Rect, + dstrect: *const sys::SDL_Rect, + angle: f64, + center: *const sys::SDL_Point, + flip: u32, + ) -> libc::c_int; + } + use crate::sys::SDL_RendererFlip::*; let mut flip = 0; if flip_horizontal { @@ -1406,7 +1422,7 @@ impl Canvas { } let ret = unsafe { - sys::SDL_RenderCopyEx( + SDL_RenderCopyEx( self.context.raw, texture.raw, match src.into() { From 3e1a370bc7912047e4656f5a95bce53e44af49d7 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 29 Apr 2024 06:55:48 +0200 Subject: [PATCH 07/13] add null terminator to string that is passed to a c function or use c string literal instead --- sdl2-sys/examples/no_std.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sdl2-sys/examples/no_std.rs b/sdl2-sys/examples/no_std.rs index f0acfdf103..16fedb751e 100644 --- a/sdl2-sys/examples/no_std.rs +++ b/sdl2-sys/examples/no_std.rs @@ -11,7 +11,9 @@ fn main() { panic!("failed to initialize sdl2 with video"); }; _window = SDL_CreateWindow( - b"hello_sdl2" as *const _ as *const i8, + // If you use Rust 1.77 or newer, you can also use a C string literal + // c"hello_sdl2".as_ptr(), + b"hello_sdl2\0".as_ptr() as *const i8, SDL_WINDOWPOS_UNDEFINED_MASK as i32, SDL_WINDOWPOS_UNDEFINED_MASK as i32, 640, From f39c9a4e363d2a243f0e12709198230441711397 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 29 Apr 2024 07:20:36 +0200 Subject: [PATCH 08/13] add comments for newly added repr(transparent) attributes --- src/sdl2/rect.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sdl2/rect.rs b/src/sdl2/rect.rs index 5a1f074ba8..c3f332045e 100644 --- a/src/sdl2/rect.rs +++ b/src/sdl2/rect.rs @@ -67,6 +67,8 @@ fn clamped_mul(a: i32, b: i32) -> i32 { /// recommended to use `Option`, with `None` representing an empty /// rectangle (see, for example, the output of the /// [`intersection`](#method.intersection) method). +// Uses repr(transparent) to allow pointer casting between Rect and SDL_Rect (see +// `Rect::raw_slice`) #[repr(transparent)] #[derive(Clone, Copy)] pub struct Rect { @@ -729,6 +731,8 @@ impl BitOr for Rect { } /// Immutable point type, consisting of x and y. +// Uses repr(transparent) to allow pointer casting between Point and SDL_Point (see +// `Point::raw_slice`) #[repr(transparent)] #[derive(Copy, Clone)] pub struct Point { From 1ae81671a11fdcc72dc42e0594c5b7f6a33d40b6 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 29 Apr 2024 10:21:45 +0200 Subject: [PATCH 09/13] fix ub when mutating with pointer made from immutable reference --- src/sdl2/rect.rs | 2 +- src/sdl2/ttf/font.rs | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/sdl2/rect.rs b/src/sdl2/rect.rs index c3f332045e..64de9ace27 100644 --- a/src/sdl2/rect.rs +++ b/src/sdl2/rect.rs @@ -485,7 +485,7 @@ impl Rect { } pub fn raw_mut(&mut self) -> *mut sys::SDL_Rect { - self.raw() as *mut _ + &mut self.raw } #[doc(alias = "SDL_Rect")] diff --git a/src/sdl2/ttf/font.rs b/src/sdl2/ttf/font.rs index d99686b912..2d7f35ab82 100644 --- a/src/sdl2/ttf/font.rs +++ b/src/sdl2/ttf/font.rs @@ -510,20 +510,21 @@ impl<'ttf, 'r> Font<'ttf, 'r> { /// Returns the glyph metrics of the given character in this font face. pub fn find_glyph_metrics(&self, ch: char) -> Option { - let minx = 0; - let maxx = 0; - let miny = 0; - let maxy = 0; - let advance = 0; + let mut minx = 0; + let mut maxx = 0; + let mut miny = 0; + let mut maxy = 0; + let mut advance = 0; + let ret = unsafe { ttf::TTF_GlyphMetrics( self.raw, ch as u16, - &minx as *const _ as *mut _, - &maxx as *const _ as *mut _, - &miny as *const _ as *mut _, - &maxy as *const _ as *mut _, - &advance as *const _ as *mut _, + &mut minx, + &mut maxx, + &mut miny, + &mut maxy, + &mut advance, ) }; if ret == 0 { From 332355d0ead063b20b3622dbecb043b970836365 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 29 Apr 2024 13:59:23 +0200 Subject: [PATCH 10/13] fix access through "too small" pointer and shrink unsafe block --- src/sdl2/pixels.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/sdl2/pixels.rs b/src/sdl2/pixels.rs index 765782197f..6f20a37ea5 100644 --- a/src/sdl2/pixels.rs +++ b/src/sdl2/pixels.rs @@ -47,14 +47,9 @@ impl Palette { // Already validated, so don't check again let ncolors = colors.len() as ::libc::c_int; - let result = unsafe { - let mut raw_colors: Vec = - colors.iter().map(|color| color.raw()).collect(); - - let pal_ptr = (&mut raw_colors[0]) as *mut sys::SDL_Color; + let colors = colors.iter().map(|color| color.raw()).collect::>(); - sys::SDL_SetPaletteColors(pal.raw, pal_ptr, 0, ncolors) - }; + let result = unsafe { sys::SDL_SetPaletteColors(pal.raw, colors.as_ptr(), 0, ncolors) }; if result < 0 { Err(get_error()) From c9c5bcb3969eb7e37488021ff12d2509a9410c21 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 29 Apr 2024 14:06:18 +0200 Subject: [PATCH 11/13] revert fix to copy_ex for now --- src/sdl2/render.rs | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/src/sdl2/render.rs b/src/sdl2/render.rs index 081eb20ab9..b0f2164f7b 100644 --- a/src/sdl2/render.rs +++ b/src/sdl2/render.rs @@ -1396,33 +1396,21 @@ impl Canvas { R2: Into>, P: Into>, { - // This function doesn't use sys::SDL_RenderCopyEx because its signature does not allow - // SDL_FLIP_HORIZONTAL | SDL_FLIP_VERTICAL for parameter 'flip'. Here, a u32 is used - // instead of SDL_RendererFlip, which does allow that value. - - extern "C" { - fn SDL_RenderCopyEx( - renderer: *mut sys::SDL_Renderer, - texture: *mut sys::SDL_Texture, - srcrect: *const sys::SDL_Rect, - dstrect: *const sys::SDL_Rect, - angle: f64, - center: *const sys::SDL_Point, - flip: u32, - ) -> libc::c_int; - } - use crate::sys::SDL_RendererFlip::*; - let mut flip = 0; - if flip_horizontal { - flip |= SDL_FLIP_HORIZONTAL as u32; - } - if flip_vertical { - flip |= SDL_FLIP_VERTICAL as u32; - } + let flip = unsafe { + match (flip_horizontal, flip_vertical) { + (false, false) => SDL_FLIP_NONE, + (true, false) => SDL_FLIP_HORIZONTAL, + (false, true) => SDL_FLIP_VERTICAL, + (true, true) => transmute::( + transmute::(SDL_FLIP_HORIZONTAL) + | transmute::(SDL_FLIP_VERTICAL), + ), + } + }; let ret = unsafe { - SDL_RenderCopyEx( + sys::SDL_RenderCopyEx( self.context.raw, texture.raw, match src.into() { From 465034236c72c3c976d637ab3a3057f511ea73f5 Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Mon, 10 Jun 2024 07:37:55 +0200 Subject: [PATCH 12/13] fix mutation through immutable pointer --- src/sdl2/ttf/font.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/sdl2/ttf/font.rs b/src/sdl2/ttf/font.rs index 2d7f35ab82..9f000752ba 100644 --- a/src/sdl2/ttf/font.rs +++ b/src/sdl2/ttf/font.rs @@ -361,18 +361,12 @@ impl<'ttf, 'r> Font<'ttf, 'r> { /// Returns the width and height of the given text when rendered using this /// font. - #[allow(unused_mut)] pub fn size_of_latin1(&self, text: &[u8]) -> FontResult<(u32, u32)> { let c_string = RenderableText::Latin1(text).convert()?; let (res, size) = unsafe { - let mut w: i32 = 0; // mutated by C code - let mut h: i32 = 0; // mutated by C code - let ret = ttf::TTF_SizeText( - self.raw, - c_string.as_ptr(), - &w as *const _ as *mut i32, - &h as *const _ as *mut i32, - ); + let mut w: i32 = 0; + let mut h: i32 = 0; + let ret = ttf::TTF_SizeText(self.raw, c_string.as_ptr(), &mut w, &mut h); (ret, (w as u32, h as u32)) }; if res == 0 { From a8fb6bdfc13cd9c055a01fedba154a72708297db Mon Sep 17 00:00:00 2001 From: Antoni Spaanderman <56turtle56@gmail.com> Date: Tue, 11 Jun 2024 18:40:48 +0200 Subject: [PATCH 13/13] update changelog --- changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/changelog.md b/changelog.md index 6235875112..6bb8a955ea 100644 --- a/changelog.md +++ b/changelog.md @@ -3,6 +3,8 @@ when upgrading from a version of rust-sdl2 to another. ### Unreleased +[PR #1389](https://github.com/Rust-SDL2/rust-sdl2/pull/1389) Fix some undefined behavior. + [PR #1368](https://github.com/Rust-SDL2/rust-sdl2/pull/1368) Remove unnecessary unsafe in Window interface. Make Window `Clone`. [PR #1366](https://github.com/Rust-SDL2/rust-sdl2/pull/1366) Add Primary Selection bindings.