From 84c57503f3fb6bae24f2f2af90b165f9a149d92d Mon Sep 17 00:00:00 2001 From: Mischa Date: Sun, 10 May 2026 21:38:23 -0700 Subject: [PATCH 1/4] feat: accept FnMut closures in enumerate_directory SDL_EnumerateDirectory is synchronous, so the callback only needs to live for the duration of the call. Switching from a bare fn pointer to a generic FnMut lets callers capture state (push into a Vec, increment a counter, etc.) without any allocation - userdata is just &mut F. Removes the EnumerateCallback type alias; existing callers passing function names or non-capturing closures keep working unchanged. --- src/sdl3/filesystem.rs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/sdl3/filesystem.rs b/src/sdl3/filesystem.rs index 74b04408..202821bd 100644 --- a/src/sdl3/filesystem.rs +++ b/src/sdl3/filesystem.rs @@ -96,14 +96,15 @@ pub fn create_directory(path: impl AsRef) -> Result<(), FileSystemError> { pub use sys::filesystem::SDL_EnumerationResult as EnumerationResult; -pub type EnumerateCallback = fn(&Path, &Path) -> EnumerationResult; - -unsafe extern "C" fn c_enumerate_directory( +unsafe extern "C" fn c_enumerate_directory( userdata: *mut c_void, dirname: *const c_char, fname: *const c_char, -) -> EnumerationResult { - let callback: EnumerateCallback = std::mem::transmute(userdata); +) -> EnumerationResult +where + F: FnMut(&Path, &Path) -> EnumerationResult, +{ + let callback = &mut *(userdata as *mut F); cstring_path!(dirname, return EnumerationResult::FAILURE); cstring_path!(fname, return EnumerationResult::FAILURE); @@ -111,17 +112,25 @@ unsafe extern "C" fn c_enumerate_directory( callback(dirname, fname) } +/// Enumerate the entries in a directory, invoking `callback` for each one. +/// +/// `SDL_EnumerateDirectory` is synchronous: it runs the callback for every +/// entry and returns before this function does, so the callback can borrow +/// state from the caller (capture by `&mut`, push into a `Vec`, etc.). #[doc(alias = "SDL_EnumerateDirectory")] -pub fn enumerate_directory( +pub fn enumerate_directory( path: impl AsRef, - callback: EnumerateCallback, -) -> Result<(), FileSystemError> { + mut callback: F, +) -> Result<(), FileSystemError> +where + F: FnMut(&Path, &Path) -> EnumerationResult, +{ path_cstring!(path); unsafe { if !sys::filesystem::SDL_EnumerateDirectory( path.as_ptr(), - Some(c_enumerate_directory), - callback as *mut c_void, + Some(c_enumerate_directory::), + &mut callback as *mut F as *mut c_void, ) { return Err(FileSystemError::SdlError(get_error())); } From 9766f6982ea78fcb3730104d1291bf2ed6649168 Mon Sep 17 00:00:00 2001 From: Mischa Date: Sun, 10 May 2026 21:45:03 -0700 Subject: [PATCH 2/4] fix: catch panics in enumerate_directory callback A panic from the user callback unwinds through SDL's extern "C" frames, which is UB. Catch it in the trampoline, stash it, and resume_unwind after SDL_EnumerateDirectory returns. Pre-existing issue on the old fn-pointer signature too, but worth fixing now that closures make it easier to hit. --- src/sdl3/filesystem.rs | 49 +++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/sdl3/filesystem.rs b/src/sdl3/filesystem.rs index 202821bd..d279a53b 100644 --- a/src/sdl3/filesystem.rs +++ b/src/sdl3/filesystem.rs @@ -96,6 +96,11 @@ pub fn create_directory(path: impl AsRef) -> Result<(), FileSystemError> { pub use sys::filesystem::SDL_EnumerationResult as EnumerationResult; +struct EnumerateState { + callback: F, + panic: Option>, +} + unsafe extern "C" fn c_enumerate_directory( userdata: *mut c_void, dirname: *const c_char, @@ -104,36 +109,50 @@ unsafe extern "C" fn c_enumerate_directory( where F: FnMut(&Path, &Path) -> EnumerationResult, { - let callback = &mut *(userdata as *mut F); + let state = &mut *(userdata as *mut EnumerateState); + if state.panic.is_some() { + return EnumerationResult::FAILURE; + } cstring_path!(dirname, return EnumerationResult::FAILURE); cstring_path!(fname, return EnumerationResult::FAILURE); - callback(dirname, fname) + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + (state.callback)(dirname, fname) + })) { + Ok(r) => r, + Err(p) => { + state.panic = Some(p); + EnumerationResult::FAILURE + } + } } /// Enumerate the entries in a directory, invoking `callback` for each one. /// -/// `SDL_EnumerateDirectory` is synchronous: it runs the callback for every -/// entry and returns before this function does, so the callback can borrow -/// state from the caller (capture by `&mut`, push into a `Vec`, etc.). +/// Runs synchronously, so `callback` may borrow caller state mutably. #[doc(alias = "SDL_EnumerateDirectory")] -pub fn enumerate_directory( - path: impl AsRef, - mut callback: F, -) -> Result<(), FileSystemError> +pub fn enumerate_directory(path: impl AsRef, callback: F) -> Result<(), FileSystemError> where F: FnMut(&Path, &Path) -> EnumerationResult, { path_cstring!(path); - unsafe { - if !sys::filesystem::SDL_EnumerateDirectory( + let mut state = EnumerateState { + callback, + panic: None, + }; + let ok = unsafe { + sys::filesystem::SDL_EnumerateDirectory( path.as_ptr(), Some(c_enumerate_directory::), - &mut callback as *mut F as *mut c_void, - ) { - return Err(FileSystemError::SdlError(get_error())); - } + &mut state as *mut EnumerateState as *mut c_void, + ) + }; + if let Some(p) = state.panic { + std::panic::resume_unwind(p); + } + if !ok { + return Err(FileSystemError::SdlError(get_error())); } Ok(()) } From 260cbb9721ff3c0752da24e904d3a3148a5de7ab Mon Sep 17 00:00:00 2001 From: Mischa Date: Sun, 10 May 2026 21:52:22 -0700 Subject: [PATCH 3/4] test: cover enumerate_directory capture, early-stop, panic, and missing path --- src/sdl3/filesystem.rs | 90 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/sdl3/filesystem.rs b/src/sdl3/filesystem.rs index d279a53b..1b8935d2 100644 --- a/src/sdl3/filesystem.rs +++ b/src/sdl3/filesystem.rs @@ -471,3 +471,93 @@ pub fn rename_path( Ok(()) } + +#[cfg(test)] +mod test { + use super::{enumerate_directory, EnumerationResult, FileSystemError}; + use std::cell::Cell; + use std::fs; + use std::path::PathBuf; + + fn temp_subdir(label: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "sdl3-rs-enumerate-{}-{}-{}", + label, + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn enumerate_directory_captures_state() { + let _sdl = crate::sdl::init().unwrap(); + let dir = temp_subdir("collect"); + fs::write(dir.join("a.txt"), b"").unwrap(); + fs::write(dir.join("b.txt"), b"").unwrap(); + + let mut names: Vec = Vec::new(); + enumerate_directory(&dir, |_d, f| { + names.push(f.to_string_lossy().into_owned()); + EnumerationResult::CONTINUE + }) + .unwrap(); + + names.sort(); + assert_eq!(names, vec!["a.txt".to_string(), "b.txt".to_string()]); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn enumerate_directory_early_stop() { + let _sdl = crate::sdl::init().unwrap(); + let dir = temp_subdir("stop"); + fs::write(dir.join("a.txt"), b"").unwrap(); + fs::write(dir.join("b.txt"), b"").unwrap(); + fs::write(dir.join("c.txt"), b"").unwrap(); + + let calls = Cell::new(0u32); + enumerate_directory(&dir, |_d, _f| { + calls.set(calls.get() + 1); + EnumerationResult::SUCCESS + }) + .unwrap(); + + assert_eq!(calls.get(), 1); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn enumerate_directory_propagates_panic() { + let _sdl = crate::sdl::init().unwrap(); + let dir = temp_subdir("panic"); + fs::write(dir.join("a.txt"), b"").unwrap(); + + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + enumerate_directory(&dir, |_d, _f| { + panic!("boom"); + }) + })); + + let payload = result.expect_err("expected panic to propagate"); + let msg = payload + .downcast_ref::<&'static str>() + .copied() + .or_else(|| payload.downcast_ref::().map(|s| s.as_str())) + .unwrap_or(""); + assert_eq!(msg, "boom"); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn enumerate_directory_nonexistent_path() { + let _sdl = crate::sdl::init().unwrap(); + let bogus = std::env::temp_dir().join("sdl3-rs-this-does-not-exist-xyz123"); + let err = enumerate_directory(&bogus, |_d, _f| EnumerationResult::CONTINUE); + assert!(matches!(err, Err(FileSystemError::SdlError(_)))); + } +} From 595337f97ab3e765e117c65b1e24075000cbb90a Mon Sep 17 00:00:00 2001 From: Mischa Date: Sun, 10 May 2026 21:59:04 -0700 Subject: [PATCH 4/4] test: harden enumerate_directory tests - Drop crate::sdl::init(); SDL_EnumerateDirectory is pure filesystem. - RAII TempDir guard so failed asserts don't leak temp directories. - Atomic counter for uniqueness instead of nanos timestamps. - Panic test now panics mid-iteration to exercise the state.panic.is_some() short-circuit in c_enumerate_directory. --- src/sdl3/filesystem.rs | 114 ++++++++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 46 deletions(-) diff --git a/src/sdl3/filesystem.rs b/src/sdl3/filesystem.rs index 1b8935d2..d0fa94d1 100644 --- a/src/sdl3/filesystem.rs +++ b/src/sdl3/filesystem.rs @@ -475,87 +475,109 @@ pub fn rename_path( #[cfg(test)] mod test { use super::{enumerate_directory, EnumerationResult, FileSystemError}; - use std::cell::Cell; use std::fs; - use std::path::PathBuf; - - fn temp_subdir(label: &str) -> PathBuf { - let dir = std::env::temp_dir().join(format!( - "sdl3-rs-enumerate-{}-{}-{}", - label, - std::process::id(), - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos() - )); - fs::create_dir_all(&dir).unwrap(); - dir + use std::path::{Path, PathBuf}; + use std::sync::atomic::{AtomicU64, Ordering}; + + /// RAII tempdir: auto-removes on drop so failed assertions don't leak. + struct TempDir(PathBuf); + + impl TempDir { + fn new(label: &str) -> Self { + static SEQ: AtomicU64 = AtomicU64::new(0); + let dir = std::env::temp_dir().join(format!( + "sdl3-rs-enumerate-{}-{}-{}", + label, + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed), + )); + fs::create_dir_all(&dir).unwrap(); + TempDir(dir) + } + + fn path(&self) -> &Path { + &self.0 + } + } + + impl Drop for TempDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } } #[test] fn enumerate_directory_captures_state() { - let _sdl = crate::sdl::init().unwrap(); - let dir = temp_subdir("collect"); - fs::write(dir.join("a.txt"), b"").unwrap(); - fs::write(dir.join("b.txt"), b"").unwrap(); + let dir = TempDir::new("collect"); + fs::write(dir.path().join("a.txt"), b"").unwrap(); + fs::write(dir.path().join("b.txt"), b"").unwrap(); - let mut names: Vec = Vec::new(); - enumerate_directory(&dir, |_d, f| { + let mut names = Vec::new(); + enumerate_directory(dir.path(), |_d, f| { names.push(f.to_string_lossy().into_owned()); EnumerationResult::CONTINUE }) .unwrap(); names.sort(); - assert_eq!(names, vec!["a.txt".to_string(), "b.txt".to_string()]); - let _ = fs::remove_dir_all(&dir); + assert_eq!(names, vec!["a.txt", "b.txt"]); } #[test] fn enumerate_directory_early_stop() { - let _sdl = crate::sdl::init().unwrap(); - let dir = temp_subdir("stop"); - fs::write(dir.join("a.txt"), b"").unwrap(); - fs::write(dir.join("b.txt"), b"").unwrap(); - fs::write(dir.join("c.txt"), b"").unwrap(); - - let calls = Cell::new(0u32); - enumerate_directory(&dir, |_d, _f| { - calls.set(calls.get() + 1); + let dir = TempDir::new("stop"); + fs::write(dir.path().join("a.txt"), b"").unwrap(); + fs::write(dir.path().join("b.txt"), b"").unwrap(); + fs::write(dir.path().join("c.txt"), b"").unwrap(); + + let mut calls = 0u32; + enumerate_directory(dir.path(), |_d, _f| { + calls += 1; EnumerationResult::SUCCESS }) .unwrap(); - assert_eq!(calls.get(), 1); - let _ = fs::remove_dir_all(&dir); + assert_eq!(calls, 1); } + /// Panics from the user callback must propagate, AND subsequent + /// invocations of the same enumeration must be suppressed (the + /// `state.panic.is_some()` short-circuit in c_enumerate_directory). #[test] - fn enumerate_directory_propagates_panic() { - let _sdl = crate::sdl::init().unwrap(); - let dir = temp_subdir("panic"); - fs::write(dir.join("a.txt"), b"").unwrap(); + fn enumerate_directory_propagates_panic_mid_iteration() { + let dir = TempDir::new("panic"); + for name in ["a.txt", "b.txt", "c.txt"] { + fs::write(dir.path().join(name), b"").unwrap(); + } + let mut seen = Vec::new(); let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - enumerate_directory(&dir, |_d, _f| { - panic!("boom"); + enumerate_directory(dir.path(), |_d, f| { + let name = f.to_string_lossy().into_owned(); + seen.push(name.clone()); + if name == "b.txt" { + panic!("boom"); + } + EnumerationResult::CONTINUE }) })); let payload = result.expect_err("expected panic to propagate"); let msg = payload - .downcast_ref::<&'static str>() + .downcast_ref::<&str>() .copied() - .or_else(|| payload.downcast_ref::().map(|s| s.as_str())) - .unwrap_or(""); - assert_eq!(msg, "boom"); - let _ = fs::remove_dir_all(&dir); + .or_else(|| payload.downcast_ref::().map(String::as_str)); + assert_eq!(msg, Some("boom")); + + // Ensure the panic short-circuit fired: no callback runs after b.txt. + // Because directory order is platform-dependent, the strongest check + // we can make is that we stopped at the panicking entry. + assert!(seen.contains(&"b.txt".to_string())); + assert_eq!(seen.last().unwrap(), "b.txt"); } #[test] fn enumerate_directory_nonexistent_path() { - let _sdl = crate::sdl::init().unwrap(); let bogus = std::env::temp_dir().join("sdl3-rs-this-does-not-exist-xyz123"); let err = enumerate_directory(&bogus, |_d, _f| EnumerationResult::CONTINUE); assert!(matches!(err, Err(FileSystemError::SdlError(_))));