Stebalien / tempfile

Temporary file library for rust
http://stebalien.com/projects/tempfile-rs
Apache License 2.0
1.18k stars 117 forks source link

Remove remove_dir_all dependency #208

Closed Xaeroxe closed 1 year ago

Xaeroxe commented 1 year ago

Fixes #129

kpark-hrp commented 1 year ago

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

Let's please update. or remove this dependency.

kpark-hrp commented 1 year ago

And release a patch version, too please.

rbtcollins commented 1 year ago

I (upstream for the crate version) would prefer to see an update. The previous objection to updating it was https://github.com/Stebalien/tempfile/pull/123#issuecomment-680302279 - 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 commented 1 year ago

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 commented 1 year ago

Build failure is https://github.com/rust-lang/rust/issues/107252.