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

Remove remove_dir_all dependency #208

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Feb 24, 2023

Fixes #129

@kpark-hrp
Copy link

@Stebalien Can we prioritize this? There is a vulnerability in this version of the library.

Let's please update. or remove this dependency.

Copy link

@kpark-hrp kpark-hrp left a comment

Choose a reason for hiding this comment

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

🔐

@kpark-hrp
Copy link

And release a patch version, too please.

@rbtcollins
Copy link
Contributor

I (upstream for the crate version) would prefer to see an update. The previous objection to updating it was #123 (comment) - which we've addressed, there is a feature 'parallel', required to opt-into the parallel deletion behaviour.

The current crate version 0.8.1 has quite small dependency list:

remove_dir_all v0.8.1 (/home/robertc/src/remove_dir_all)
├── cfg-if v1.0.0
├── cvt v0.1.1
│   └── cfg-if v0.1.10
├── fs_at v0.1.0
│   ├── cfg-if v1.0.0
│   ├── cvt v0.1.1 (*)
│   ├── libc v0.2.139
│   └── nix v0.26.2
│       ├── bitflags v1.3.2
│       ├── cfg-if v1.0.0
│       ├── libc v0.2.139
│       └── static_assertions v1.1.0
├── lazy_static v1.4.0
├── libc v0.2.139
└── normpath v1.1.0

And if its a dominating concern I'd be happy to trim it further. The fs_at MSRV might be an issue; I'd be happy to see how far back I can push it - I don't think there are concrete concerns, but solving the TOCTOU issues was a higher priority than figuring out how fare back I could easily go. Going further back than 1.58.1 would just re-open the TOCTOU concern though :/.

If you do remove it, thats fine - could I ask (and I'd obviously be willing to put up a patch) for a feature to opt-into the remove_dir_all crate? The parallel deletion support makes a huge difference to rustup's uninstall performance, and I've been meaning to improve our development experience by getting the same performance there :)

@Stebalien
Copy link
Owner

Yep, this seems reasonable. I'll merge and release today.

In terms of adding a feature to enable this, that sounds reasonable. I'm also willing to enable it by default if it ends up causing performance issues for users, but I'm somewhat skeptical that'll be an issue in practice (temporary directories tend to be short lived and therefore tend not to have tons of files).

@Stebalien
Copy link
Owner

Build failure is rust-lang/rust#107252.

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.

Depend on remove_dir_all 0.6
4 participants