Provide a way of detecting system library leakage by papertigers · Pull Request #5259 · oxidecomputer/omicron

papertigers · 2024-03-13T20:41:29Z

After #5158 was integrated @Rain noticed that attempting to run a build of omdb in the switch zone suddenly stopped working and filed oxidecomputer/helios-omicron-brand#15. @jgallagher ended up fixing this by splitting out the sled-hardware types into their own crate in #5245.

We decided it would be good if we added some sort of CI check to omicron to catch these library leakages earlier. This PR introduces that check and adds it to the helios build and test buildomat job. I have also added some notes to the readme for others that may end up adding a new library dependency.

Locally I modified the allow list so that it would produce errors, those errors end up looking like:

$ cargo xtask verify-libraries
    Finished dev [unoptimized + debuginfo] target(s) in 0.42s
     Running `target/debug/xtask verify-libraries`
    Finished dev [unoptimized + debuginfo] target(s) in 4.11s
Error: Found library issues with the following:
installinator
	UNEXPECTED dependency on libipcc.so.1
omicron-dev
	UNEXPECTED dependency on libipcc.so.1
	UNEXPECTED dependency on libresolv.so.2
sp-sim
	UNEXPECTED dependency on libipcc.so.1
	UNEXPECTED dependency on libresolv.so.2
sled-agent
	NEEDS libnvme.so.1 but is not allowed
mgs
	UNEXPECTED dependency on libipcc.so.1
	UNEXPECTED dependency on libresolv.so.2


If depending on a new library was intended please add it to xtask.toml

Break's wicketd's dependency on libnvme

sunshowers · 2024-03-14T00:53:16Z

+        let entry = entry.context("dirent")?;
+        if !entry.file_type()?.is_dir() {
+            continue;
+        }


I think we should be building all of the binaries here -- cargo build --bins should do it. You can use the JSON metadata with --message-format json-render-diagnostics, then iterate over the built binaries to check for them.

I realize it's a significant chunk of work, but it is hopefully not a massive amount, and will make this check much more resilient.

Oh, and I forgot to mention that cargo_metadata, which xtask already pulls in, has support for this: https://docs.rs/cargo_metadata/0.18.1/cargo_metadata/enum.Message.html#method.parse_stream

Thanks for the tip! Fixed in 276e135

sunshowers · 2024-03-14T00:56:33Z

 }

+fn cmd_verify_library() -> Result<()> {
+    let metadata = load_workspace()?;


Probably should have a check here to error out if this is run on a non-illumos platform. (I'd recommend a stub impl that just prints out an error like "error: this command is only supported on illumos")

Fixed in dd71b07

sunshowers

putting it back in your queue!

sunshowers

Looks fantastic, thanks! Just a few minor comments.

sunshowers · 2024-03-19T21:53:51Z

@@ -0,0 +1,144 @@
+use anyhow::{bail, Context, Result};


MPL files require a license header up top -- could you add one here?

Fixed in fa5b905

sunshowers · 2024-03-19T21:54:37Z

+fn verify_executable(
+    config: &XtaskConfig,
+    path: &Utf8Path,
+    errors: &mut HashMap<String, Vec<LibraryError>>,


Should be some kind of ordered map -- either a BTreeMap or an IndexMap (probably a BTreeMap)

Fixed in fa5b905

sunshowers · 2024-03-19T21:56:41Z

+    config_path.push(".cargo/xtask.toml");
+    let config = read_xtask_toml(&config_path)?;
+
+    let mut command = Command::new("cargo")


This should use $CARGO if available (which it will be almost all of the time, since cargo is invoked via the rustup wrapper)

Fixed in fa5b905

I left it as an error if the env::var("CARGO") fails, but happy to fall back on just "cargo" if you like that better.

Should definitely fall back to cargo, yeah. This pattern is pretty standard, and is expressed in bash as e.g. ${CARGO:-cargo}.

sunshowers · 2024-03-19T21:57:17Z

+        let mut msg = String::new();
+        use std::fmt::Write;
+        errors.iter().for_each(|(binary, errors)| {
+            write!(msg, "{binary}\n").unwrap();


writeln!

Also you can use https://crates.io/crates/swrite to avoid the unwrap.

Fixed in fa5b905

sunshowers · 2024-03-19T21:59:07Z

+# If we are running on illumos we want to verify that we are not requiring
+# system libraries outside of specific binaries. If we encounter this situation
+# we bail.
+# NB: `cargo xtask verify-libraries` runs `cargo build --bins` to ensure it can


Doesn't look like it runs cargo build --bins? Just cargo build.

Fixed in fa5b905

jgallagher and others added 10 commits March 12, 2024 13:36

add sled-hardware-types crate

97c484e

Break's wicketd's dependency on libnvme

add sled-hardware-types crate

c0cfacb

Break's wicketd's dependency on libnvme

remove phantom libnvme dependencies in debug builds

4dddd26

cargo fmt

adb563b

actually commit sled-hardware-types

f6c32d3

Provide a way of detecting system library leakage

2445f20

Merge remote-tracking branch 'origin/main' into mike/lib-verify

8efc76f

Add timeout to buildomat job

8decf7c

Add some documentation

c8791e9

Typo

8029cbe

papertigers mentioned this pull request Mar 13, 2024

Add libnvme.so.1 to omicron zones oxidecomputer/helios-omicron-brand#15

Closed

papertigers marked this pull request as ready for review March 13, 2024 22:32

papertigers requested review from jgallagher and sunshowers March 13, 2024 22:33

Check the exit status of elfedit

4e31ae8

sunshowers reviewed Mar 14, 2024

View reviewed changes

Comment thread dev-tools/xtask/src/main.rs Outdated

sunshowers requested changes Mar 15, 2024

View reviewed changes

Comment thread library-verification.toml Outdated

papertigers added 2 commits March 18, 2024 20:49

Address Rain's feedback

276e135

Add a stub impl for non illumos systems

dd71b07

papertigers requested a review from sunshowers March 18, 2024 21:04

papertigers added 3 commits March 18, 2024 21:46

Pull illumos bits into a module to make clippy happy

da25241

Mark fn pub

c5ac263

timeout expects external commands

b78e056

sunshowers approved these changes Mar 19, 2024

View reviewed changes

sunshowers reviewed Mar 19, 2024

View reviewed changes

Another round of Rain feedback and cleanup

fa5b905

papertigers requested a review from sunshowers March 20, 2024 16:39

papertigers added 2 commits March 20, 2024 18:31

Merge remote-tracking branch 'origin/main' into mike/lib-verify

c886aea

Fallback to "cargo" if $CARGO is unset

c2b849d

papertigers enabled auto-merge (squash) March 20, 2024 20:46

papertigers merged commit 5d97551 into main Mar 22, 2024

papertigers deleted the mike/lib-verify branch March 22, 2024 02:02

Uh oh!

Conversation

papertigers commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sunshowers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sunshowers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

papertigers commented Mar 13, 2024 •

edited

Loading