brson / rust-sdl

SDL bindings for Rust
MIT License
179 stars 52 forks source link

Fixed sdl::get_error to not segfault #155

Closed TheNeikos closed 9 years ago

TheNeikos commented 9 years ago

This one was a bit tricky as I am not sure what the initial version had tried to do. SDL_GetError() returns a constant char pointer. (char const *) However they are already in bytes. c_str_to_bytes expects a pointer to a c_str as seen here: Rust Source

This version seems safer to me as we do not transmute, and since SDL explicitely states that those strings are fixed in memory this should not segfault.

TheNeikos commented 9 years ago

And to what this fixes.

This was my previous test setup:

extern crate sdl;
extern crate sdl_image;

use std::rand::Rng;

use sdl::video::{SurfaceFlag, VideoFlag};
use sdl::event::{Event, Key};

#[main]
pub fn main() {
    sdl::init(&[sdl::InitFlag::Video]);
    sdl_image::init(&[sdl_image::InitFlag::PNG]);
    sdl::wm::set_caption("rust-sdl demo - video", "rust-sdl");

    let mut rng = std::rand::thread_rng();
    let screen = match sdl::video::set_video_mode(800, 600, 32,
                                                  &[SurfaceFlag::HWSurface],
                                                  &[VideoFlag::DoubleBuf]) {
        Ok(screen) => screen,
        Err(err) => panic!("failed to set video mode: {}", err)
    };

    let img_path = Path::new("./test.png");
    let image = match sdl_image::load(&img_path) {
        Ok(surface) => surface,
        Err(err) => panic!("Failed to load image: {}", err)
    };

    screen.blit(&image);
    screen.flip();

    'main : loop {
        'event : loop {
            match sdl::event::poll_event() {
                Event::Quit => break 'main,
                Event::None => break 'event,
                Event::Key(k, _, _, _)
                    if k == Key::Escape
                        => break 'main,
                _ => {}
            }
        }
    }

    sdl::quit();
}

It segfaults without this fix, and correctly panics with.

lgrz commented 9 years ago

Thanks for tracking down this issue. You're absolutely right, the calls to transmute look like they were left over from previous changes and unnecessary, unless I'm missing something.

I think we can still use c_str_to_bytes as I don't think we need to call strlen directly, Rust does that for us in c_str_to_bytes.

It seems that SDL_GetError (SDL 1.2) returns a char * and not a const char *, so coercing the result of SDL_GetError to a *const c_schar or similar should do the trick. Thoughts?

TheNeikos commented 9 years ago

You are right about SDL_GetError returning a char*, however I am wondering about cleanup, is it like in SDL2 a static string or are we supposed to free it?

lgrz commented 9 years ago

Yes it's a static string like in SDL2.

TheNeikos commented 9 years ago

Okay, I have switched to using c_str_to_bytes I also tested it and it seems to work as intended.

lgrz commented 9 years ago

I think we can remove the strlen import otherwise it looks good to me. Thanks!

TheNeikos commented 9 years ago

Oh right, forgot to remove that, on it!

lgrz commented 9 years ago

Thanks again!