-
Notifications
You must be signed in to change notification settings - Fork 476
Description
Description
There is a potential soundness issue in several drivers()
iterator functions across the crate. These functions call their corresponding SDL_GetNum...Drivers()
FFI functions and directly use the return value as a length, assuming it will always be non-negative.
This issue has been identified in the following locations:
src/sdl2/video.rs:2296
, invideo::drivers()
callingSDL_GetNumVideoDrivers()
src/sdl2/render.rs:3279
, inrender::drivers()
callingSDL_GetNumRenderDrivers()
src/sdl2/audio.rs:438
, inaudio::drivers()
callingSDL_GetNumAudioDrivers()
For example, the implementation in video.rs
includes a comment that explicitly states this incorrect assumption:
// SDL_GetNumVideoDrivers can never return a negative value.
DriverIterator {
length: unsafe { sys::SDL_GetNumVideoDrivers() },
index: 0,
}
However, the official SDL2 documentation for these functions states that they can return a negative value on failure. For SDL_GetNumVideoDrivers
:
(int) Returns a number >= 1 on success or a negative error code on failure; call SDL_GetError() for more information.
The current implementation in the wrapper directly contradicts this API contract.
Impact
This discrepancy can lead to Undefined Behavior (UB).
- If an underlying SDL function fails (e.g., due to a driver issue or misconfiguration), it will return a negative integer like
-1
. - This negative
int
will be reinterpret-cast to an unsigned integer (u32
orusize
) for thelength
field of the iterator. - The value
-1
will becomeu32::MAX
orusize::MAX
, resulting in an iterator that believes it has a massive number of items. - Any attempt to use this iterator (e.g., in a
for
loop or by calling.next()
) will immediately try to access an out-of-bounds index, triggering Undefined Behavior. This could result in a panic, a hard crash, or a potential security vulnerability.
This is a critical soundness bug as unsafe
code is failing to uphold its safety invariants.
Suggested Fix
The return value from the unsafe
FFI call must be checked before it is used to initialize the iterator. The same fix pattern should be applied to all three identified functions (video::drivers
, render::drivers
, and audio::drivers
).
A robust implementation should:
- Call the
SDL_GetNum...Drivers()
function and store theint
result. - Check if the result is less than 0.
- If it is negative, handle the error gracefully. Returning an empty iterator is a safe and reasonable default. The SDL error could also be logged for debugging purposes.
- If the result is non-negative, proceed to cast it to the appropriate unsigned integer type and create the iterator.
Here is a pseudo-code example for video::drivers()
:
pub fn drivers() -> DriverIterator {
// Call the unsafe function and store the result
let num_drivers = unsafe { sys::SDL_GetNumVideoDrivers() };
if num_drivers < 0 {
// On failure, handle the error by returning a safe, empty iterator.
// Optionally, log the error from SDL.
// sdl2::log::log_error(sdl2::log::Category::Video, &sdl2::get_error());
DriverIterator { length: 0, index: 0 }
} else {
// On success, it is now safe to cast and create the iterator.
DriverIterator {
length: num_drivers as u32, // Or the correct unsigned type for the struct
index: 0,
}
}
}
Thank you for maintaining this great library.