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

Add analysis of unsafe code in dependencies through cargo geiger #72

Merged
merged 7 commits into from
Jun 22, 2021

Conversation

nasifimtiazohi
Copy link
Contributor

@nasifimtiazohi nasifimtiazohi commented Jun 21, 2021

Changes made

Add processing of cargo geiger report for unsafe code

Improvement TODO

  1. As Cargo geiger cannot run over a virtual manifest and need to run over a package.toml, our code runs cargo geiger over all member packages making it a very expensive and time consuming process. Issue reported in the upstream.

xvschneider
xvschneider previously approved these changes Jun 21, 2021
Copy link

@xvschneider xvschneider left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link

@metajack metajack left a comment

Choose a reason for hiding this comment

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

Looks good although I think we can hide the cache from caller code a bit and make it easier to get reports instead of having to test keys and stuff everywhere the reports might get used.


fn get_unsafe_report(&self, name: String, version: String) -> Result<Option<UnsafeReport>> {
let key = (name, version);
let cache = self.geiger_cache.borrow();

Choose a reason for hiding this comment

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

Maybe there should be a get_cargo_geiger_report that grabs one from the cache based on the key? That would hide the cache a bit from the other code.

Choose a reason for hiding this comment

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

Probably it would return Option<CargoGeigerReport>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants