Got bitten by Drop impl running to early when passing TempDir to fn(p: impl AsRef<Path>)

[robinhundt]

Yesterday i got badly bitten by using tempdir() in conjunction with std::process::Command::current_dir(...) . My initial code looked like this:

let tmp_dir = tempdir();
let output = Command::new(bin)
    .current_dir(tmp_dir)
    .output()?;  

with bin doing file operations inside tmp_dir. Unfortunately this did not work, and I received an error along the lines of Os Error 2: No such file or directory.
After beating my head against the screen for some time i thought "maybe the tmp_dir is dropped to early and the directory deleted before bin is run?" so i changed

    .current_dir(tmp_dir)

to

    .current_dir(&tmp_dir)

and my program worked just as intended.
Like the issue title says, this was due to the signature of

pub fn current_dir<P: AsRef<Path>>(&mut self, dir: P) -> &mut Command

Since TempDir implements AsRef<Path> i thought that moving it in would be the right thing to do (i didn't need to access the tmp_dir later in the method after constructing the command). But this lead to Drop impl of TempDir being run after current_dir finished but before the execution of bin.

This kind of mistake can quickly happen, especially since many APIs dealing with Paths are generic over AsRef<Path>.
My question is if this pitfall should be pointed out in the documentation and if so, where the best place would be to do this? I'd like to start contributing to some rust crates and this seems like a really easy thing to do, but which could have saved me some time and nerves :D

I was also thinking if it's possible to prevent this mistake, maybe by changing the AsRef<Path> impl to:

impl AsRef<Path> for &TempDir

but this would be a breaking change.

Edit: I assume all of this applies to TempFile as well.

[Stebalien]

This is an interesting issue. You're right that changing the impl to &TempDir (and &mut TempDir) would fix this, the real question is, as you say, how much would it break. Implementing AsRef<Path> on TempDir by value is, in theory at least, a useful feature: structs can store the reference to the temporary directory without caring about what it actually is, and have it cleaned up when they're done with it.

On the other hand, nearly all AsRef users immediately call as_ref and convert the path into an owned path if they actually need to store it. I've never actually seen a struct that's generic over something implementing AsRef<Path>.

For now, I'd document this pitfall in the top-level lib.rs (add a section under "Security").

---

Note: Your fix works but there's an edge-case: Rc, Arc, etc. There's currently a blanket impl of AsRef<Path> on, e.g., Arc<T> for all T implementing AsRef<Path>. Currently, that means Arc<TempDir> implements AsRef<Path>. Unfortunately, if we switch the AsRef impl over to &Path, we'd need to explicitly implement AsRef<Path> for &Arc<TempDir> but we're not allowed to do that.

[Stebalien]

(on the other hand, autoderef probably takes care of that in most cases)

[marcospb19]

I fell to this pitfall as well, took me a lot of debugging hours because I assumed the error was somewhere else, it's usually something you do not expect.

Because TempDir is sensitive to Drop pitfalls, I think that the AsRef<Path> impl should be removed for TempDir and NamedTempFile so that other people don't fall into this too.

[bnjbvr]

I've been bitten by this too, and spent a few hours debugging a failing test because of this. Based on a colleague's suggestion, I'm wondering if the AsRef<Path> impl could exist only for &TempDir instead of TempDir?

[jplatte]

Currently, that means Arc<TempDir> implements AsRef<Path>.

Do people put TempDirs in Arcs in practice?

[Stebalien]

@jplatte https://grep.app/?q=Arc%3CTempDir%3E

I'll keep this issue in mind if we ever have a v4 (and can introduce large breaking changes), but it's not enough to motivate a v4.