Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safe APIs should be safe, unsafe APIs should be marked unsafe #1095

Closed
langston-barrett opened this issue Feb 23, 2023 · 5 comments
Closed
Labels
bug Something isn't working

Comments

@langston-barrett
Copy link
Contributor

Describe the bug

I can write a program without using unsafe that segfaults.

To Reproduce

$ cat > libafl/examples/unsafe.rs <<EOF
use libafl::prelude::{tuple_list, ListObserver, MatchName, TimeObserver};

fn main() {
    let ot = tuple_list!(TimeObserver::new("name"));
    let lo = ot.match_name::<ListObserver<()>>("name").unwrap();
    println!("{}", lo.list().len());
}
EOF

$ cargo run --quiet --example unsafe

zsh: segmentation fault (core dumped)  cargo run --example unsafe

$ git rev-parse HEAD

64a57ad3e3802cfff711c4b6dd8f42b8c19bfd01

Expected behavior

The Rust convertion is that any program that doesn't use unsafe should not be subject to memory corruption. In cases where the library can't guarantee this, the API in question should be marked unsafe.

Additional context

I've phrased this issue pretty generally, though I've reported a very specific instance. The reason is: I think there's a lot of unsafe code in LibAFL, and I wonder if there are other places where this expectation is violated.

The relevant code for the specific problem noted above is here:

/// Returns if the type `T` is equal to `U`
#[rustversion::not(nightly)]
#[must_use]
pub const fn type_eq<T: ?Sized, U: ?Sized>() -> bool {
// BEWARE! This is not unsafe, it is SUPER UNSAFE
true
}

@langston-barrett langston-barrett added the bug Something isn't working label Feb 23, 2023
@domenukk
Copy link
Member

I think in this specific case (and probably most cases) it's better to try and build a safe wrapper.
But in general you are right of course!

@addisoncrump
Copy link
Collaborator

Can we replace this with type_id?

@andreafioraldi
Copy link
Member

Can we replace this with type_id?

No because it enforces 'static. This is unsafe only in stable, while safe with nightly rust. It is more of a WIP API, ofc we can mark it unsafe but will be safe in the future

@domenukk
Copy link
Member

domenukk commented Aug 3, 2023

This specific example is fixed by removing the unsafety (mostly)
Other examples still persist across the codebase, I add some needed unsafe specifiers in #1398

@domenukk
Copy link
Member

Closing this for now, I think we are at a better state now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants