jeaye / ncurses-rs

A low-level ncurses wrapper for Rust
Other
678 stars 101 forks source link

Segmentation fault when calling function `scr_restore` #222

Open cicilzx opened 4 days ago

cicilzx commented 4 days ago

Hi, I'm developing a fuzzer to test the safe abstraction, and I fonnd a segmentation fault when calling function scr_restore.

Below are two inputs that can trigger the segmentation fault. I think is because missing the pre-condition or post-condition checking when calling the unsafe function ll::scr_restore.

Input-1:

use ncurses::scr_restore;

fn main() {
    let filename = ".";
    let _ = scr_restore(filename);
}

Input-2:

use ncurses::scr_restore;

fn main() {
    let filename = "/";
    let _ = scr_restore(filename);
}

The output is:

zsh: segmentation fault  cargo run
correabuscar commented 3 days ago

With initscr^1 it doesn't crash on scr_restore but rather, later, on a refresh or getch for example. But the crashing happens due to the ncurses lib itself not being able to handle edge-cases like the filename being the current dir (.), not much this ncurses-rs wrapper could do about it, I guess, except sanitize the inputs but since it's meant to be only a thin wrapper ... unlikely this would happen?!

This is a very thin wrapper around the ncurses TUI lib. NOTE: The ncurses lib is terribly unsafe and ncurses-rs is only the lightest wrapper it can be. If you want a safe and idiomatic TUI library for Rust, look elsewhere. If you want a 1:1 port of C to Rust or you want to crank a TUI out C-style in Rust, this will probably do the trick.

(from readme)

ie. the fault lies within the ncurses lib itself. Consider reporting them upstream, you can look here^2 for info.

cicilzx commented 3 days ago

Thank you!

cicilzx commented 2 days ago

I have asked the developers of ncurses about this issue, and bellow I quote the developer's response:

In a quick check, I see that from scr_restore returns ERR for this case, but that (according to the trace) an fopen on a directory is returning a valid FILE* (which fails to give usable data, causing the SCREEN to be freed and leading to a crash on the next refresh(). Some systems will disallow that fopen, but Linux isn't doing that.

While I can (and will) add a check to ensure that's a file (rather than a directory), it won't fix the larger problem of feeding it bad input and the rust binding not protecting the user by doing something with the error status. You'll have to check the return value for stuff like this.

I think it would be very nice to add a precondition to check the bad input (e.g., checking if that's a file and it's readable by current), as you mentioned in issue 223. It would imporve the experice of using this lib for all users. Thank you so much for the comments.