rust-secure-code / cargo-auditable

Make production Rust binaries auditable
Apache License 2.0
646 stars 28 forks source link

Can't build recent `gitoxide` versions #124

Open jcgruenhage opened 1 year ago

jcgruenhage commented 1 year ago

https://github.com/Byron/gitoxide doesn't successfully build with cargo-auditable anymore. It fails in https://github.com/rust-secure-code/cargo-auditable/blob/9fa7fb3988b4d2414397b9550a3f3fd1aa1bc8f2/cargo-auditable/src/collect_audit_data.rs#L77-L80. I'm not sure if gitoxide or cargo-auditable is to blame here, and the error message isn't all that helpful to me either:

thread 'main' panicked at 'cargo metadata failure: error: Package `gix-features v0.31.0 (/home/jcgruenhage/dev/github.com/Byron/gitoxide/gix-features)` does not have feature `prodash`. It has an optional dependency with that name, but that dependency uses the "dep:" syntax in the features table, so it does not have an implicit feature with that name.
', cargo-auditable/src/collect_audit_data.rs:77:9

That reads to me as if something tried to enable a prodash feature on gix-features, but I couldn't find such a thing.

For context, afaict, v0.23.0 is the last version that builds. Anything newer than that runs into the above issue. I ran into this while trying to update the version of gitoxide we ship on Void Linux.

Shnatsel commented 1 year ago

The error originates in cargo metadata command that is part of the official Cargo distribution, and that cargo auditable runs to get information about the crates.

I can reproduce the issue on the latest version of the repo from git and Rust 1.70. I will investigate further. Thank you for the report!

jcgruenhage commented 1 year ago

Thanks for looking into this, reporting it is the least I can do when seeing something like this ;)

Shnatsel commented 1 year ago

It seems to be a bug in cargo. It passes --cfg 'feature="prodash"' to rustc even though there is no such feature in the crate, only a dep: declaration. Then, when cargo auditable extracts the list of features passed to rustc and feeds it back to cargo, cargo itself reports an error because the feature it passed to rustc doesn't actually exist.

Unfortunately this is something that will have to be fixed in Cargo, so I cannot provide an ETA for the fix. I will make a minimal reproducing example and submit the issue upstream.

Shnatsel commented 1 year ago

I've filed an issue upstream: https://github.com/rust-lang/cargo/issues/12336

But the turnaround on Cargo issues is not very quick, so I might have to work around this somehow - perhaps with another call to cargo metadata to get the list of actually existing features, and filter the --cfg 'feature=foo' directives against that? That will slow down the build but at least it will work.

alexandrevicenzi commented 1 year ago

I have the same issue building Wasmtime.

The conflict seems to be related to https://github.com/rust-lang/cargo/issues/10788.

The build succeeds without cargo-auditable.

Are there any workarounds for this issue that do not include disabling cargo-auditable?

alexandrevicenzi commented 1 year ago

I found a workaround that seems to work.

Patched Wasmtime.

-wasi = ['wasi-cap-std-sync', 'wasmtime-wasi', 'cap-std', 'wasi-common']
+wasi = ['wasi-cap-std-sync', 'wasmtime-wasi', 'dep:cap-std', 'wasi-common']
JohnRTitor commented 4 months ago

Any update on this? It affects other packages as well, like lightningcss https://github.com/parcel-bundler/lightningcss/issues/702.

Shnatsel commented 4 months ago

Unfortunately this is a bug in Cargo itself, and AFAIK there is nothing I can really do to fix that in cargo auditable.

The proper way to do this would be in Cargo, or to integrate cargo auditable functionality into Cargo itself making the external tool obsolete. Sadly the progress on upstreaming is slow - the prerequisite https://github.com/rust-lang/rfcs/pull/3553 is not yet merged (although a prototype implementation is underway) and the actual RFC for cargo auditable-like functionality has been recently postponed.

JohnRTitor commented 4 months ago

I don't really understand, it works in cargo, but with auditable it errors out. Usual workaround is to remove dep: from the features table.

Shnatsel commented 2 months ago

https://github.com/rust-lang/rfcs/pull/3553 would be the proper, correct solution to this problem.

Until then, there may be hope for working around Cargo passing nonexistent features to rustc, which causes these failures.

When compiling a crate, Cargo sets the current working directory of the child rustc process to the crate’s directory. Therefore, we could discover the Cargo.toml file for the crate, parse it, extract a list of all actually existing features, and filter the features passed by cargo to rustc against that list.

However, that ties cargo auditable to the evolving Cargo.toml format with a lot of complexity and edge cases. We might be able to bypass parsing it ourselves by invoking cargo metadata --offline --no-deps and parsing its output.

bjorn3 commented 2 months ago

If you run cargo metadata directly on the Cargo.toml of the dependency, that will generate/update a Cargo.lock file, clobbering the cached source for the crate.

Shnatsel commented 2 months ago

Fortunately that is not a problem if I run cargo metadata --no-deps, which is what I am going to do. This creates its own problems - it is difficult to determine what package the Cargo.toml actually belongs to, which is silly. Starting with Cargo 1.71 I can use the default_workspace_members field, but for earlier versions I will probably need some cursed heuristics if I want to keep --no-deps.

Besides that, I will only need to run cargo metadata on workspace members, and all workspace members already have their Cargo.lock generated or the build would not proceed. So I could actually afford to run without --no-deps, if I'm willing to take the compilation time hit.

Shnatsel commented 2 months ago

I have prototyped a fix in the https://github.com/rust-secure-code/cargo-auditable/tree/fix-dep-features branch. Tests pass, but I don't have a way to check if that fixes the issue because the latest versions of gitoxide, wasmtime and lightningcss build successfully even without these changes.

Shnatsel commented 2 months ago

Okay, I managed to reproduce this on gitoxide if I check out v0.24.0 tag. My "fix" doesn't actually fix it. Debugging time!

Shnatsel commented 2 months ago

Okay, it seems that cargo metadata is buggy and reports that gix-features has the feature prodash when it actually doesn't, it just has a dep:prodash syntax without the actual feature.

I don't think I can work around the issue in cargo auditable when there are this many different bugs in Cargo around it :face_with_diagonal_mouth: