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

Expressions in unsafe functions does not count as unsafe #71

Closed
faern opened this issue Oct 4, 2019 · 7 comments · Fixed by #72
Closed

Expressions in unsafe functions does not count as unsafe #71

faern opened this issue Oct 4, 2019 · 7 comments · Fixed by #72
Labels
bug Something isn't working

Comments

@faern
Copy link

faern commented Oct 4, 2019

The following code makes cargo-geiger report 1 unsafe function and 0 unsafe expressions:

unsafe fn danger() {
    5 / 3;
}

But if I change it into the following, it is reported as 1 unsafe function and 1 unsafe expression (but with a warning about superfluous unsafe of course):

unsafe fn danger() {
    unsafe { 5 / 3; }
}

Both contain the exact same amount of expressions that could potentially be unsound. Both libraries are equally unsafe. But cargo-geiger thinks the latter is more unsafe.

I think cargo-geiger need to count every expression in unsafe fns as unsafe expressions, because they are allowed to do anything you can do in unsafe Rust.

Another point to show the same thing. If I refactor the following:

unsafe fn just_slightly_dangerous() {
    let careful_arg = safe();
    watch_out(careful_arg);
}

Into:

fn just_slightly_dangerous() {
    let careful_arg = safe();
    // SAFETY: We know `careful_arg` upholds the requirements of `watch_out` because {...}
    unsafe { watch_out(careful_arg); }
}

Then cargo-geiger reports that I have one unsafe function less, but more unsafe expressions than before. While in reality I have proven to the compiler and the reader that safe() is indeed safe, everything else is the same as before. I made fewer expressions compile as unsafe Rust. This does not feel like a very fair way of counting the unsafety of my library.

@anderejd
Copy link
Contributor

anderejd commented Oct 4, 2019

Thank you for the report! I will find the time to read it and reply during the weekend.

@anderejd
Copy link
Contributor

anderejd commented Oct 4, 2019

Excellent examples, this is clearly a bug. Traversing into an unsafe function during scanning should put the scanner context into the "count unsafe" state during the entire function scope. Would you be interested in making a PR?

@anderejd anderejd added the bug Something isn't working label Oct 4, 2019
@anderejd
Copy link
Contributor

anderejd commented Oct 4, 2019

Looking at the code makes me think there's another bug related to unsafe scopes:

https://github.com/anderejd/cargo-geiger/blob/8e42785dd3966b678f03fb5e5c9bb3959d7b2ed8/geiger/src/lib.rs#L129

The in_unsafe_block should be a integer that gets incremented for each nested unsafe block, including unsafe function scopes. ...and probably called something like nested_unsafe_scopes.

anderejd added a commit that referenced this issue Nov 5, 2019
@anderejd anderejd mentioned this issue Nov 5, 2019
anderejd added a commit that referenced this issue Nov 5, 2019
@faern
Copy link
Author

faern commented Nov 5, 2019

Awesome @anderejd, thanks!

@anderejd
Copy link
Contributor

anderejd commented Nov 6, 2019

No problem, thank you for the excellent bug report! :)

@anderejd
Copy link
Contributor

Released in 0.8.

@faern
Copy link
Author

faern commented Nov 19, 2019

Awesome, thanks!
Now my number of unsafe expressions skyrocketed. But at least they are more correct, and they at least go down, instead of up, when I make an unsafe fn into a safe one with only a few unsafe expressions :)

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

Successfully merging a pull request may close this issue.

2 participants