Skip to content

FFI Unsoundness in drivers() functions, risking Undefined Behavior #1499

@zeroy0410

Description

@zeroy0410

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, in video::drivers() calling SDL_GetNumVideoDrivers()
  • src/sdl2/render.rs:3279, in render::drivers() calling SDL_GetNumRenderDrivers()
  • src/sdl2/audio.rs:438, in audio::drivers() calling SDL_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).

  1. If an underlying SDL function fails (e.g., due to a driver issue or misconfiguration), it will return a negative integer like -1.
  2. This negative int will be reinterpret-cast to an unsigned integer (u32 or usize) for the length field of the iterator.
  3. The value -1 will become u32::MAX or usize::MAX, resulting in an iterator that believes it has a massive number of items.
  4. 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:

  1. Call the SDL_GetNum...Drivers() function and store the int result.
  2. Check if the result is less than 0.
  3. 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.
  4. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions