#![forbid(unsafe_code)] not detected when depending on a library that has a binary without #![forbid(unsafe_code)]

[TyPR124]

Example repository: https://github.com/TyPR124/geigertest

There is a lib, "testlib", which contains #![forbid(unsafe_code)], but a binary (still in "testlib") that does not.

There is also a separate binary, "testbin", which depends on "testlib". In this case, I do not believe there is any possiblity that testbin would be able to see or use unsafe code from testlib (because such code doesn't exist in the lib, only its bin), so ideally cargo geiger, when run on testbin, should detect this and report that testlib includes #![forbid(unsafe_code)] (because it does).

[anderejd]

Sounds reasonable, thanks for reporting!

[anderejd]

One of the places that needs to change is here: https://github.com/anderejd/cargo-geiger/blob/71cf546cf19ee6bd1e0802514936dad927684b2e/cargo-geiger/src/cli.rs#L370-L380

Should a crate be classified as "forbids unsafe code" if some entry points allow unsafe?

[TyPR124]

I thought about this some more, and I think these are some things to consider regarding forbids_unsafe requirements:

1. cargo geiger appears to always ignore examples. An example can contain unsafe code and it has no effect on cargo geiger, regardless of if the crate is the top-level crate or a dependency. Probably 99% of the time, any executable included with a library should be an example.

2. I think it would be possible to depend on a libraries bundled executable in some way, though not directly by code. I've never tired this or seen it done (as far as I know), but for example a program might want to, at runtime, run an executable that was bundled with one of its dependencies.

The second point is a bit of a grey area for me. On one hand, unsafe code in a foreign executable does not affect the main program's memory safety. On the other hand, the if the foreign executable acts strangely, it might negatively impact the main program in another way. In that case, knowing that said foreign executable contains unsafe code might be helpful in troubleshooting.

I think based on this, it probably makes most sense to continue as is, not granting "forbids unsafe code" if a libraries binary contains unsafe code. While it may cause confusion (it did for me), it's also airing on the side of caution and providing as much useful information as possible.

With all that said, I think a feature that would both reduce confusion and increase usefulness of output would be to measure instances of unsafe independently for binaries that are bundled in other crates. So for the example repo I provided, it might look like:

0/0        0/0          0/0    0/0     0/0      ?  testbin 0.1.0
0/0        0/0          0/0    0/0     0/0      ?  └── testlib 0.1.0
0/0        0/0          0/0    0/0     0/0      ?      └── bin 0.1.0 (bundled binary)

I have no idea if this is feasible to implement or what kind of work it would take to do so (or if it is even truly a good idea), but if you think it is feasible and are willing to provide a bit of guidance, I'd be willing to give it a shot.

[anderejd]

To collect unsafe code metrics from binaries and libraries in each crate would require building each binary in addition to the library, which would increase build (scan) times. It's doable, but I'm not yet convinced it's what we want.

Reading through your comments makes me lean more towards changing the requirements for the crate level "forbids unsafe" status. The current requirements for the crate level "forbids unsafe" is that all entry points in the crate must declare #![forbid(unsafe_code)]. What I'm leaning towards is what I think you were originally asking for and that is: The library entry point for dependencies must declare #![forbid(unsafe_code)] but binary entry points are allowed to not do so. The root crate entry point must declare the forbid attribute regardless of if it's a binary or a library. What do you think about that?

[TyPR124]

would require building each binary in addition to the library

To be honest I assumed they were being built already since they were included in the forbids unsafe requirements. Maybe instead that could be something to do if an optional flag is present? Regardless...

The library entry point for dependencies must declare #![forbid(unsafe_code)] but binary entry points are allowed to not do so. The root crate entry point must declare the forbid attribute regardless of if it's a binary or a library.

I think this is still a good approach, and is what I had in mind originally.

If the root crate is a library which includes binaries, do those binaries get built and/or count towards "forbids unsafe"?

[anderejd]

If the root crate is a library which includes binaries, do those binaries get built and/or count towards "forbids unsafe"?

I think the default entry point is enough, unless a specific one is specified.

[anderejd]

would require building each binary in addition to the library

To be honest I assumed they were being built already since they were included in the forbids unsafe requirements. Maybe instead that could be something to do if an optional flag is present? Regardless...

I'm pretty sure the the bin targets are not built for library dependencies. Do you have an example where that happens?

[TyPR124]

Do you have an example where that happens?

No, sorry I had the thought process of "it sees the binaries aren't forbidding unsafe code, therefore it must be building said binaries", but evidently that isn't how this tool works. Just an incorrect assumption on my part.

[anderejd]

Okay. Yes, the entry points are discovered by the cargo library part of cargo-geiger, without the need to build them. All that's needed to decide if a target forbids unsafe is parse its entry point Rust file and look for the attribute. To figure out if a source file is used by the build the other hand, the entire crate needs to be built by rustc, for now at least.

[pinkforest]

It's a bug #93

Cargo de-coupling and CLI options mirror will fix this for 0.12.0