Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rust] dependency upgrades to fix cargo audit warnings #19203

Merged
merged 7 commits into from
Aug 7, 2023

Conversation

nbdd0121
Copy link
Contributor

Fix (almostly) #18970

  • Update nix to 0.26
    • requires some small code changes
  • Update serialport to 4.2.1
    • requires patch update
  • Switch from atty to is-terminal
    • requires minor code updates
  • cargo update rest of dependencies

This addresses half of our cargo-audit warnings. The rest are:

  • time, the affected version is still a dependency of chrono which mdbook uses, but the affected API is not used.
  • mach, dependency of serialport but is only used on MacOS
  • ansi_term, dependency of serde-annotate

Since time and mach does not affect us, I suppressed their warnings by adding relevant security advisory to audit.toml ignore section.

This lefts us with just ansi_term.

@nbdd0121 nbdd0121 requested review from cfrantz and a team as code owners July 14, 2023 12:01
@nbdd0121 nbdd0121 requested a review from dmcardle July 14, 2023 12:07
@nbdd0121
Copy link
Contributor Author

Blocked until #19208 goes in.

@dmcardle
Copy link
Contributor

Hi Gary, thanks for working on this!

Out of curiosity, how have you been running cargo audit?

Copy link
Contributor

@dmcardle dmcardle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

index 65c42bb..ee50f55 100644
index bec8f86...0000000 100644
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @lschuermann, now I remember you mentioned that this patch didn't apply cleanly. Thanks @nbdd0121 for updating it!

@nbdd0121
Copy link
Contributor Author

Hi Gary, thanks for working on this!

Out of curiosity, how have you been running cargo audit?

I just cd into third_party/rust and run cargo audit from there.

@nbdd0121 nbdd0121 force-pushed the dep branch 2 times, most recently from 831c871 to 0140c37 Compare July 27, 2023 16:51
The output file is slightly different, likely due to the Bazel version
upgrade.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
The current nix version has security advisory RUSTSEC-2021-0119.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
The current version is yanked.

Updating the version makes the patch not applicable, so
the patch is updated and rebased.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
@nbdd0121 nbdd0121 force-pushed the dep branch 2 times, most recently from 661a1ae to baa7099 Compare August 3, 2023 16:56
@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Aug 3, 2023

The previous CI failure is caused by serde-derive starting to use precompiled binary but the file is not considered as data file in Rust rules so Bazel excludes them from sandbox. As a short-term solution I pinned Bazel to 1.0.171 which is the last version without precompiled binary,

Comment on lines 58 to 60
# serde-derive 1.0.172 uses precompiled binary which does not work well with Bazel.
serde = { version="=1.0.171", features = ["derive"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create an issue in lowRISC/opentitan for this and leave a TODO comment here:

# TODO(opentitan#xxxxx): serde-derive 1.0.172 uses ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #19362

lschuermann pushed a commit to lschuermann/opentitan that referenced this pull request Aug 3, 2023
`atty` is unmaintained. Switch to `is-terminal` crate, which is already
one of our transitive dependency, and has the same API as the Rust std
1.70 `IsTerminal` trait.

There are a few places that we are using `nix` to do `isatty`, and
these are migrated as well.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
serde 1.0.172+ tries to use precompiled binary for proc macro but
Bazel's Rust rules do not include them as data files so build breaks.

This also removes the explicit serde_derive dependency which is
not needed because we don't access serde_derive crate directly.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
The update addresses the following cargo-audit warnings:
* `hermit-abi`, which is a transitive dependency of a few crates,
  is updated to an unyanked version.
* `h2`, which is a transitive dependency of mdbook, is updated to
  an unyanked version.

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
This adds `.cargo/audit.toml` which suppresses two crates that are
flagged by `cargo audit` but does not affect us:
* `time`, dependency of `chrono`, affected API is not used
* `mach`, dependency of `serialport`, only affects MacOS

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
@nbdd0121 nbdd0121 added the Status:Ready to merge PR is ready to be merged by a committer. label Aug 7, 2023
@vogelpi vogelpi merged commit 111a439 into lowRISC:master Aug 7, 2023
cfrantz pushed a commit to cfrantz/opentitan that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Ready to merge PR is ready to be merged by a committer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants