Unsafe blocks review

[pinkforest]

Hi considering this crate is getting popular 🥳 congrats great work ~80k a day from less than a month :)

Just an idea as this crate is becoming very important in the ecosystem ..

or FWIW considering I am deeply interested about unsafe across the whole Rust 🦀 ecosystem I wanted to chime in :)

Have you considered / or had anyone to vet these unsafe blocks in this crate properly through externally like some neutral 3rd party not deep in the project / associated crates ? I didn't see anything in crev or safety-dance.

You could also just hold back merging anything that touches any unsafe {} and put a label like Unsafe-Proposed and let it sit for a week unless it is critical urgent change - so the community has the chance to chime in - that's what I do in advisory-db to radiate my intent especially on controversial advisories that are not necessarily as time critical -

By radiating intent everyone feels having a chance to say something that leads less to haphazard changes people feeling included

Also I find easiest raising issues first and then doing a PR when people either agree with me or we refine the idea / issue first

[astraw]

Thanks for the thoughts and pointers.

I would be happy for someone to vet the unsafe blocks. How would you suggest we facilitate that?

The Unsafe-Proposed label also seems like a good idea.

We have become so "low level" that in some cases we cannot rely on higher level libraries, for example #50 and #61.

[pinkforest]

Great - We can organise this via safety-dance - I've added issue at rust-secure-code/safety-dance#79

[lopopolo]

One bit that I've been thinking about is that the windows and macOS internals are implemented by big unsafe fn. I don't think that's quite right. The functions are not unsafe to call (there's no invariants callers must uphold).

Instead, the functions seem to be declared as unsafe fn to avoid having to use and document unsafe blocks in their internals.

[lopopolo]

I would like to find find a way to fail CI when the following lints fire:

• unsafe_op_in_unsafe_fn
• clippy::undocumented_unsafe_blocks

[pinkforest]

For clippy you would have to add it into github action - that undocumented lint was stabilised I think

unsafe_op I am not sure it was stabilised so might need +nightly for cargo clippy

You can put thhem into lib.rs as #[deny(..)] if they were not default.. I think undocumented was warn by default

You could gate any nightly-needed features so you don't suddenly require nightly for complication - e.g. gate them via nightly feature and then use that feature in CI

[lopopolo]

@pinkforest ahh I meant getting the code changed so the lints would pass.

For flipping the lints on, my preference would be to set them as warnings either in lib.rs or on the CLI invocations of clippy and cargo check in CI with RUSTFLAGS="-D warnings" in the env for all GitHub Actions jobs.

[Kijewski]

Instead, the functions seem to be declared as unsafe fn to avoid having to use and document unsafe blocks in their internals.

As the person who wrote the functions: You're right, I did not want to wrap every other line in unsafe.

[pinkforest]

Closing as question answered - thanks!

[astraw]

@pinkforest many thanks, I think this really improved the crate! We will endeavor to keep up the high standard and please feel free to remind us of this goal in the future. :)