Macro expansion

[anderejd]

Add macro expansion of some kind. Perhaps directly using cargo expand or doing something similar.

[mxxo]

Hi! I think this would 100% take care of the plutonium issue.
For functions like:

#[safe]
unsafe fn real_safe(x: f32) -> i32 {
    std::mem::transmute::<f32, i32>(x)
}

#[safe]
fn deref_null() {
    let p = &mut 0 as *mut i32;
    println!("{}", *p);
}

If you call expand, you get something like this:

fn real_safe(x: f32) -> i32 {
    #[allow(unused_unsafe)]
    unsafe {
        std::mem::transmute::<f32, i32>(x)
    }
}
fn deref_null() {
    #[allow(unused_unsafe)]
    unsafe {
        let p = &mut 0 as *mut i32;
        // etc  ....
    }
}

So I think the regular function visitor can handle it after expansion.
I don't think the disclaimer where the expanded code /= actual code applies here, but maybe it's just something to watch out for.

[Shnatsel]

Turns out there is a way to leverage rustc to detect all uses of unsafe code without requiring nightly: https://www.reddit.com/r/rust/comments/g9mw57/oneliner_to_correctly_list_all_uses_ofunsafe_in/

My quick tests show that it does in fact detect unsafe code in macros.

[anderejd]

Looks promising! Anyone who reads this and want to add it or something similar to cargo-geiger, please go ahead :)

EDIT: As in go ahead and send a PR ;)

[eddyb]

As a quick explanation, the pieces are:

• passing -vv to Cargo means that it will use --cap-lints=warn instead of --cap-lints=allow, so that you can get lints from dependencies without having to run rustc yourself
  • the otherwise redundant --cap-lints=warn in RUSTFLAGS means that crates in the current workspace will also get the same behavior, so that no fatal errors will be caused by lints
• --message-format=json allows you to parse and filter Cargo/rustc messages
  • I use jq in the example but you'd likely use serde_json from Rust, ofc
  • the rendered field gives you full user-friendly rustc output, so no information is lost
  • you also get more details about the source location in JSON, even a full macro backtrace, so you can do more interesting statistics
• -Funsafe-code in RUSTLAGS forces the lint you're looking for
  • this generalizes to any rustc or even clippy lints
  • -D (deny) and -W (warn) are equivalent given --cap-lints=warn, but my hope was that -F (forbid) couldn't be overridden by #[allow(unsafe_code)] in the Rust code, as usually forbid isn't, but I haven't confirmed this
    • it's possible --cap-lints=warn replaces -F with -W eagerly, which we might want to fix in rustc if that's the case

[8573]

I well may be misunderstanding something, but it occurs to me that expanding a proc macro means executing arbitrary code, potentially allowing an adversarial proc macro to hide unsafe by messing with the running cargo-geiger process (edit: or merely not emitting unsafe if it sees that cargo-geiger is running). (Of course there are worse things it could do, but I suppose those aren't the focus of this ticket.)

(The state of cargo-sandbox is unfortunate.)

[carbotaniuman]

The adversarial proc-macro can already hide unsafe code currently though, so we're no better off, and I given that some proc macros do legitimately use unsafe code that should be audited, I think the benefits are worth it.

[tarcieri]

Yeah, a more straightforward scenario is a proc macro that detects it's being run under cargo-geiger and emits one set of code in that scenario without unsafe, but otherwise would use unsafe

[8573]

It occurs to me to speculate (idly) that cargo-geiger itself could try to hide from proc-macros, maybe by changing its process name to (e.g.) "cargo-make" and (where available) using bind-mount trickery to hide .travis.yml etc. if these mention "geiger" ... I'm not suggesting this should be a development priority. :-)

[Shnatsel]

Then we would be engaging in an arms race with attackers, except attackers know our single exact set of behaviors and we know nothing about the attackers. This is doomed from the start.

[carbotaniuman]

I was mainly addressing macro expansion for things like the old proposal for cxx or stuff wrapping unsafe operations like pin_mut, not to protect against stuff like plutonium (which I really don't have a problem with).

[anderejd]

Slightly related to #69, since #69 will replace the cargo crate with cargo as a subprocess.