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

(fix) Bump remove_dir_all to 0.8.2 #42

Merged
merged 3 commits into from
Apr 9, 2023

Conversation

barathrm
Copy link
Contributor

@barathrm barathrm commented Apr 8, 2023

Old version has a listed security vuln.

Fixes

Crate:     remove_dir_all
Version:   0.6.1
Title:     Race Condition Enabling Link Following and Time-of-check Time-of-use (TOCTOU)
Date:      2023-02-24
ID:        RUSTSEC-2023-0018
URL:       https://rustsec.org/advisories/RUSTSEC-2023-0018
Solution:  Upgrade to >=0.8.0
Dependency tree:
remove_dir_all 0.6.1
└── etebase 0.5.3

Copy link
Contributor

@Xiretza Xiretza left a comment

Choose a reason for hiding this comment

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

Looks like remove_dir_all doesn't really follow semver, there were no breaking changes between 0.6.1 and 0.8.0 that I can find. So a pure bump without changing any code should be fine.

@tasn
Copy link
Member

tasn commented Apr 9, 2023

CI is failing.

@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch from a9c8d72 to ef92d9e Compare April 9, 2023 10:29
@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Cargo build succeeded locally, but the test fails with not finding version 0.8.0 of remove_dir_all. Trying with 0.8.2.

@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch from ef92d9e to 4f2e7ae Compare April 9, 2023 10:36
@tasn
Copy link
Member

tasn commented Apr 9, 2023

I think you have a typo? You added a v

@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch from 4f2e7ae to 411f3a5 Compare April 9, 2023 10:43
@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Yep, fixed 🤦

@tasn
Copy link
Member

tasn commented Apr 9, 2023

Still failing. :|

@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Still failing. :|

Hm. Seems like the part that's failing is the test running cargo with rust nightly and -Z minimal-versions. I can look into why that might fail (a quick read of the docs (?) doesn't really help me), but if someone knows that'd be a nice short-cut. xD

@tasn
Copy link
Member

tasn commented Apr 9, 2023

There are also unrelated clippy errors (I guess because we are using a new enough clippy) which will just be fixed with clippy --fix ,but probably deserve a new PR. Would be great if you could do that too, though otherwise I can quickly do it myself. LMK.

@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Running the nightly toolchain locally with cargo generate-lockfile -Z minimal-versions ¹ works fine for me. A bit of googling indicates that this kind of thing might happen if cargo uses a cache to look up dependencies?

¹ edit: followed by cargo test --no-fail-fast -- --test-threads=1

@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

There are also unrelated clippy errors (I guess because we are using a new enough clippy) which will just be fixed with clippy --fix ,but probably deserve a new PR. Would be great if you could do that too, though otherwise I can quickly do it myself. LMK.

Attempt here #43

@Xiretza
Copy link
Contributor

Xiretza commented Apr 9, 2023

Nightly is only used to generate the minimal-versions lockfile, the tests themselves run with the Minimum Supported Rust Version (1.56). Maybe remove_dir_all 0.8.0 requires a newer MSRV?

@tasn
Copy link
Member

tasn commented Apr 9, 2023

@barathrm, I merged your other PR, please rebase this on top of that so clippy is fixed. :)

Old version has a listed security vuln.
@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch from 878afd6 to 34bc024 Compare April 9, 2023 11:48
@tasn
Copy link
Member

tasn commented Apr 9, 2023

I think @Xiretza is right. We can update the min rust version for all I care. I didn't even remember we set one.

@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Nightly is only used to generate the minimal-versions lockfile, the tests themselves run with the Minimum Supported Rust Version (1.56). Maybe remove_dir_all 0.8.0 requires a newer MSRV?

You're right, I can reproduce the issue locally by manually using 1.56.0. Gonna try bumping to something newer.

@barathrm barathrm changed the title (fix) Bump remove_dir_all to 0.8.0 (fix) Bump remove_dir_all to 0.8.2 Apr 9, 2023
@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch from 0bd8356 to df2cb4d Compare April 9, 2023 12:22
@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Not sure why the minimal versions workflow fails on openssl now... can't reproduce that locally, and the docker does have libssl-dev installed.

@tasn
Copy link
Member

tasn commented Apr 9, 2023

No idea. :|

@Xiretza
Copy link
Contributor

Xiretza commented Apr 9, 2023

Wild guess: the container image has updated/changed at some point and now contains an incompatible openssl version?

@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Wild guess: the container image has updated/changed at some point and now contains an incompatible openssl version?

Hm, maybe. Or the versions selected by rust nightly's -Z minimal-versions aren't compatible with the docker's openssl?

Seems like the workflow uses ubuntu-latest, which I assume is the default github ubuntu latest docker.

@Xiretza
Copy link
Contributor

Xiretza commented Apr 9, 2023

Hm, maybe. Or the versions selected by rust nightly's -Z minimal-versions aren't compatible with the docker's openssl?

Those shouldn't have changed since the last time CI ran (successfully).

Seems like the workflow uses ubuntu-latest, which I assume is the default github ubuntu latest docker.

It's probably that then - ubuntu-latest always points at the latest release, which can and will change. Looks like it updated to openssl 3.x, which isn't supported by openssl-sys 0.9.55: https://stackoverflow.com/questions/67346713/wsl-ubuntu-cargo-rust-openssl-environment-variables-not-set

@tasn
Copy link
Member

tasn commented Apr 9, 2023

Should we just bump openssl-sys?

@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch 2 times, most recently from 6f6a596 to e01a5a6 Compare April 9, 2023 14:05
haadr and others added 2 commits April 9, 2023 16:32
1.63.0 is low enough to be in debian sid, but new enough to have
  remove_all_dir 0.8.2.
Also bump cc, required to build newer openssl-sys.
@barathrm barathrm force-pushed the barathrm/fix-remove-dir-dep branch from e01a5a6 to da12fd6 Compare April 9, 2023 14:33
@barathrm
Copy link
Contributor Author

barathrm commented Apr 9, 2023

Well then. bumped openssl-sys, openssl and cc. Had to bump cc to be able to build the newer openssl-sys, as mentioned in the commit msg. Tests pass.

@tasn tasn merged commit cf88433 into etesync:master Apr 9, 2023
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.

4 participants