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

Forbid unsafe code #432

Closed
ralpha opened this issue Sep 19, 2020 · 3 comments · Fixed by #434
Closed

Forbid unsafe code #432

ralpha opened this issue Sep 19, 2020 · 3 comments · Fixed by #434

Comments

@ralpha
Copy link

ralpha commented Sep 19, 2020

structopt does not seem to use unsafe code, which is very good :)

To prevent this from ever happening, add following line to the top:

#![forbid(unsafe_code)]

of https://github.com/TeXitoi/structopt/blob/master/src/lib.rs
and https://github.com/TeXitoi/structopt/blob/master/structopt-derive/src/lib.rs

This will make the package green in
https://github.com/rust-secure-code/cargo-geiger

@CreepySkeleton
Copy link
Collaborator

To prevent this from ever happening

I think it worth mentioning that being over-zealous against unsafe is as harmful as the other extreme.

The point of cargo geiger is to highlight the fact that a select version of a crate does or does not use unsafe, so the person auditing the code immediately knows which crates need special attention. #[forbid(unsafe_code)] is not a promise to never use unsafe code, it's a sign that this particular version does not use unsafe code - not a policy. I can't imagine why we would ever want unsafe in structopt though.

But if I find a strong incentive to use unsafe in a crate of mine, I will seriously consider using it even if unsafe has been forbidden. Unsafe is a tool, not a black plague, and even though it sure has its own drawbacks as any tool does, it's programmer's responcibility to consider the pros and cons and the risks and come up with a weighted (hahahahaha) decision.

(Disclaimer: structopt is not my crate, @TeXitoi is the the big guy here whom I admire and respect. He makes this kind of calls.)

@TeXitoi
Copy link
Owner

TeXitoi commented Sep 23, 2020

That's to prevent to happen by accident.

Unsafe is a tool, to be used mainly in ffi, low level code and performance optimization. I never had to use the later, and I write at work mainly CPU bound programs. I definitively think structopt will never need unsafe.

@ralpha
Copy link
Author

ralpha commented Sep 24, 2020

@CreepySkeleton I agree that unsafe has its place. But having a small barrier to add 1 line of unsafe code to an otherwise clean project is always good. Makes you think twice if it is really needed. So just because someone did not want to take a good look at the problem, or having just one line in of unsafe just to shave of 0.1ms of speed is not what we want (in most cases). And yet it is sometimes used this way.

I also don't expect structops to use unsafe code, so why not add the line (as you did, thanks :) ).
Unsafe code sometimes turns up in weird places. Like colored (for colors in the terminal) https://github.com/mackwic/colored has unsafe code because of detecting it the code is inside a terminal or virtual terminal on windows, so uses windows API. Did not expect to see unsafe code there at first, but there is. (maybe it is not even needed, have not checked, just picked something here)

As @TeXitoi pointed out most low level or FFI code use unsafe functions (as it is just needed sometimes). But for most higher level crates it is not really needed.
So better just forbid it then to accidentally get it into the code-base.

So yes I agree, and changing the geiger list from red/white to green while having the same functionality is also not bad 😄 . Having unsafe code has its downsides.

Like this has to have at least some memory issues with all that unsafe code. But al least regex-syntax is safe ;)
Screenshot from 2020-09-24 04-32-13

Thanks for adding the line/fix so quickly (and even having a new shiny badge I see 😉 ) and thanks for the wonderful crate too! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants