Tracker: All unsafe blocks must be removed and then re-added one by one after careful verification of actual safety.
Opened this issue · 12 comments
Using the FFI is unsafe, but simply putting unsafe { } around the call is not enough to get rid of the problem. It's unsafe for a reason. It's unsafe because it can do horrible things and cause UB.
You need to adjust the library so that only functions that cannot possibly cause UB, no matter what, are wrapped in an unsafe block and marked as safe. All other functions must be left as unsafe functions, hopefully with explanations in the docs of what to do to avoid problems.
To make this issue a bit more actionable:
The goal of the crate is to expose a thin wrapper on top of C API. This is fine as long as the safety invariants are encoded in the type system:
// This is fine
pub unsafe fn do_stuff() {
call_c_code();
}
// This is not OK - it violates Rust's memory safety guarantees, see
// https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html
pub fn do_stuff() {
unsafe {
call_c_code();
}
}This crate currently follows the latter patter, which is contrary to Rust's safety encapsulation mechanisms. See https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html
@Lokathor could you provide an example of this in the current code and how it goes wrong in practice?
This has been discussed before. Please refer to this: #187 (comment)
Specifically:
I would be open to a PR marking all of the appropriate fns unsafe, but not any more PRs or issues around making ncurses-rs do anything more than its purpose.
Every single FFI call must be left as unsafe unless you can demonstrate that it is safe. The default is that it's always unsafe.
I'm not saying that you need to create any grand framework, just remove all the unsafe blocks and change all the function signatures.
I'm not saying that you need to create any grand framework, just remove all the unsafe blocks and change all the function signatures.
I know. My point is that ncurses-rs was written 5 years ago, when Rust was around version 0.10.0, its usage and patterns were much less defined, and I certainly didn't know them very well. Since then, it's been maintained by the community and has not been touched by me beyond merging PRs.
I think it makes sense to mark all fns unsafe now, but I don't use Rust at this point and I don't use ncurses-rs. I'd be happy to merge a PR which makes this change, even if it'd be breaking.
If we found a home for the crate with someone who actively uses Rust would you be interested in transferring ownership to an active Rust user?
I'd be happy to merge a PR which makes this change, even if it'd be breaking.
@jeaye May I suggest having one issue open as a tracking issue, and explicitely marking it 'Contributions wanted' or whathever label your prefer? That way this is somewhat more explicit than a mere note in the Readme. It would also allow for better tracking. A separate branch where parts of the safety reviews are collected could restrict the impact to a single breaking release even if multiple people and changes are involved (and provide a target on which to depend when safety is immediately important).
That said, the wording of these two issues was not very helpful from the standpoint of another contributor. It's not entirely clear what needs to be addressed, and even less clear how. It also leaves open what specifically the end result should look like. @Lokathor The initial post would be relevant as motiviation and partial explanation but is not sufficiently actionable for contributions.
@HeroicKatora Well, from what I recall of ncurses (also haven't used it in a year or two) nearly every single function can segfault you if you didn't at least call init_scr first, so... yeah, step 1 is to just remove 100% of the unsafe blocks in the entire crate and start fresh, deciding when to mark something safe after an analysis.
If we found a home for the crate with someone who actively uses Rust would you be interested in transferring ownership to an active Rust user?
I think we can start with PRs, then add that person as a collaborator, and then transfer ownership once that person has shown to be dedicated enough. I'm not comfortable transferring ownership of a crate with nearly 5K downloads a month to just the next guy who says he's interested in helping.
May I suggest having one issue open as a tracking issue, and explicitely marking it 'Contributions wanted' or whathever label your prefer?
Done.
step 1 is to just remove 100% of the unsafe blocks in the entire crate and start fresh, deciding when to mark something safe after an analysis.
Pretty much. Most fns should be unsafe. Anything taking in or returning pointers, which isn't everything, but is a whole lot.
I agree that we should not hand the crate over to a total rando. More that I was looking to see if you'd be open to the idea at all. These days in the Rust 1.37 era we've actually developed a few group dedicated to passively maintaining crates of people who have moved on from Rust.
Until then, we've got a less argumentative title.
The purpose this crate serves is now fulfilled by -sys crates that expose unsafe fns, and then another crate creates an actually safe wrapper on top of that.
FWIW there is another crate providing low-level ncurses bindings through bindgen already, as well as a safe wrapper on top. Perhaps this is an opportunity to work together and avoid duplication of effort.