Skip Menu |
 

This queue is for tickets about the File-Path CPAN distribution.

Report information
The Basics
Id: 31721
Status: resolved
Priority: 4/
Queue: File-Path

People
Owner: dland [...] cpan.org
Requestors: alan [...] pair.com
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 2.04
Fixed in: 2.05



Subject: rmtree: "evil this way comes" unnecessarily
Date: Wed, 19 Dec 2007 13:58:15 -0500 (EST)
To: bug-File-Path [...] rt.cpan.org
From: Alan Ferrency <alan [...] pair.com>
Download (untitled) / with headers
text/plain 1.6k
Hello, I was inadvertantly able to invoke the fatal error "cannot chdir to [parent-dir] from [child-dir]: [errmsg], aborting." error, in a fairly trivial way. In short: if your cwd is inside the tree you're running rmtree() on, the error above will occur. I don't know whether this should be considered a bug in the code, a bad error message for this particular case, or an instance of "don't do that, it's dumb" (which may want to be mentioned in the pod alongside "evil this way comes.") I'd consider updating the code to use modern error handling, but this is happening implicitly when File::Temp's cleanup() calls rmtree(), and I don't have control over File::Temp. In any case, I know my real solution is "don't do that, it's dumb," which I will implement now that I understand the problem. A short .t, below, demonstrates the problem. On perl 5.10 and File::Path 2.04, I get: Failed 5/7 tests, 28.57% okay Failed Test Stat Wstat Total Fail List of Failed ------------------------------------------------------------------------------- t/6_file_path.t 5 1280 7 5 1-2 5-7 Thanks! Alan Ferrency pair Networks, Inc. alan@pair.com #--------------- use strict; use warnings; use Test::More tests => 7; use File::Path; my $test_dir = "/tmp/foo"; ok !-e $test_dir, "ensure our test directory doesn't exist"; ok mkdir($test_dir, 777), "make test directory"; ok mkdir("$test_dir/bar", 777), "make test subdirectory"; ok chdir("$test_dir/bar"), "enter test subdirectory"; ok eval { rmtree($test_dir) }, "rmtree the test directory"; ok !$@ or diag "rmtree died: $@", "no error in rmtree"; ok !-e $test_dir, "test directory doesn't exist"; #------------------
From: dland [...] cpan.org
On Wed Dec 19 13:58:37 2007, alan@pair.com wrote: Show quoted text
> Hello, > > I was inadvertantly able to invoke the fatal error "cannot chdir to > [parent-dir] from [child-dir]: [errmsg], aborting." error, in a fairly > trivial way. > > In short: if your cwd is inside the tree you're running rmtree() on, > the > error above will occur.
Ah yes, indeed it will. This is fall-out from the race condition fix. Consider a huge directory tree under /foo/bar/rat/quux, and delete from ./quux onwards. If another process moved quux to under foo directly, rmtree could have begun to delete the contents of bar as well. Show quoted text
> I don't know whether this should be considered a bug in the code, a > bad > error message for this particular case, or an instance of "don't do > that, it's dumb" (which may want to be mentioned in the pod alongside > "evil this way comes.")
I think it's safe to say that it falls under the banner of "doctor, it hurts when I do this". Then again, if you can do it, so can someone else. A first level of defence would be to ask whether the cwd is the same as the directory to be rmtree'ed. In which case, we just have to set 'keep_root' to 1 and let the code go its merry way. Perhaps spitting out a warning at the same time. I'll have to think about whether there's anything sane that can be done if someone wants to do the equivalent of rmtree('../../') Show quoted text
> I'd consider updating the code to use modern error handling, but this > is > happening implicitly when File::Temp's cleanup() calls rmtree(), and I > don't have control over File::Temp. In any case, I know my real > solution > is "don't do that, it's dumb," which I will implement now that I > understand the problem.
I'll get in touch with the current File::Temp maintainer and see if they can make it smarter about deleting from the parent instead of itself. Show quoted text
> A short .t, below, demonstrates the problem. On perl 5.10 and > File::Path
A bug report with a test suite, bless you sir! I'll make use of it in some way. Thanks for the report, I'll see what I can do. Regards, David Landgren
Subject: Re: [rt.cpan.org #31721] rmtree: "evil this way comes" unnecessarily
Date: Thu, 20 Dec 2007 10:54:00 -0500 (EST)
To: David Landgren via RT <bug-File-Path [...] rt.cpan.org>
From: Alan Ferrency <alan [...] pair.com>
Download (untitled) / with headers
text/plain 1.3k
On Thu, 20 Dec 2007, David Landgren via RT wrote: Show quoted text
> <URL: http://rt.cpan.org/Ticket/Display.html?id=31721 > >
> > I'd consider updating the code to use modern error handling, but > > this is happening implicitly when File::Temp's cleanup() calls > > rmtree(), and I don't have control over File::Temp. In any case, I > > know my real solution is "don't do that, it's dumb," which I will > > implement now that I understand the problem.
> > I'll get in touch with the current File::Temp maintainer and see if they > can make it smarter about deleting from the parent instead of itself.
Well, it was me, not File::Temp, which was chdir'ing into the temp directory, so I'm pretty sure this isn't their fault/problem. But in this case, it feels questionable whether I did something truly unreasonable or not... only once I understood why it failed, did I consider it particularly bad. From my perspective, it looked like: - create a temporary directory with File::Temp - chdir into the directory - do stuff - exit It was only during File::Temp's implicit "cleanup" step at END time that the problem showed up. Show quoted text
> > A short .t, below, demonstrates the problem. On perl 5.10 and > > File::Path
> > A bug report with a test suite, bless you sir!
No problem :) That's how I found the exact issue that I originally thought was in my code... Alan Ferrency
From: MSCHILLI [...] cpan.org
Download (untitled) / with headers
text/plain 590b
I had a similar issue, looks like there's two possible solutions if someone does something like this: mkpath("/tmp/foo/bar"); chdir("/tmp/foo/bar"); rmtree("/tmp/foo"); which will cause the somewhat confusing cannot chdir to /tmp/foo/bar from /tmp/foo: error: 1) Report the error up-front 2) Change the cwd() to the directory containing the tree to be rmtree()d. While 2) would be the most convenient, it might be confusing to the user who all of a sudden ends up in a different directory without warning. As for 1), please find a simple fix attached. -- Mike Schilli
Download patch.txt
text/plain 570b
--- File-Path-2.04/Path.pm 2007-11-13 03:54:49.000000000 -0800 +++ File-Path-2.04.patched/Path.pm 2008-02-07 13:01:18.000000000 -0800 @@ -177,6 +177,13 @@ }; for ($arg->{cwd}) { /\A(.*)\Z/; $_ = $1 } # untaint + for my $path (@$paths) { + if($arg->{cwd} =~ /^$path/) { + _error($arg, "cannot delete $path while cwd is $arg->{cwd}"); + return 0; + } + } + @{$arg}{qw(device inode)} = (stat $arg->{cwd})[0,1] or do { _error($arg, "cannot stat initial working directory", $arg->{cwd}); return 0;
Subject: Re: [rt.cpan.org #31721] rmtree: "evil this way comes" unnecessarily
Date: Thu, 7 Feb 2008 16:14:04 -0500 (EST)
To: Michael_Schilli via RT <bug-File-Path [...] rt.cpan.org>
From: Alan Ferrency <alan [...] pair.com>
Download (untitled) / with headers
text/plain 975b
If it is possible to detect "Perl is in the END {} times" (in the process of shutting everything down), then I would be more likely to prefer #2 at least in in this case. But really it seems likely more trouble than it's worth. Alan On Thu, 7 Feb 2008, Michael_Schilli via RT wrote: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=31721 > > > I had a similar issue, looks like there's two possible solutions if > someone does something like this: > > mkpath("/tmp/foo/bar"); > chdir("/tmp/foo/bar"); > rmtree("/tmp/foo"); > > which will cause the somewhat confusing > > cannot chdir to /tmp/foo/bar from /tmp/foo: > > error: > > 1) Report the error up-front > 2) Change the cwd() to the directory containing the tree to be rmtree()d. > > While 2) would be the most convenient, it might be confusing to the user > who all of a sudden ends up in a different directory without warning. As > for 1), please find a simple fix attached. > > -- Mike Schilli >
From: dland [...] cpan.org
Download (untitled) / with headers
text/plain 1.3k
On Thu Feb 07 16:36:20 2008, alan@pair.com wrote: Show quoted text
> If it is possible to detect "Perl is in the END {} times" (in the > process of shutting everything down), then I would be more likely to > prefer #2 at least in in this case. But really it seems likely more > trouble than it's worth.
The infrastructure is there to do it: END { f() } sub f { my @c = caller(1); print "called by $c[3]\n"; } One would just have to walk up the call stack and see if there's an END block there. What makes me hesitate is symlinks. If you're in a child directory that is symlinked in from elsewhere, asking to rmtree() the parent might not delete what you thought it would, or worse, it might delete what you thought it wouldn't. I guess it depends on whether the fact that rmtree() will delete a symlink rather than trying to read it is what you expected. My personal opinion, not that it amounts to much, is a feeling that it is wrong to try and delete your own directory out from under you. Where are you afterwards? Still, that's two people bitten in a few months. There's obviously something not right with the interface as things stand. I think I would rather add a flag 'updir' => 1 which would instruct rmtree to chdir up out out the directory tree it was being asked to delete. If you want to do something semantically ambiguous I think you should say so. David
Subject: Re: [rt.cpan.org #31721] rmtree: "evil this way comes" unnecessarily
Date: Thu, 7 Feb 2008 18:36:18 -0500 (EST)
To: David Landgren via RT <bug-File-Path [...] rt.cpan.org>
From: Alan Ferrency <alan [...] pair.com>
Download (untitled) / with headers
text/plain 4.1k
Hello again, On Thu, 7 Feb 2008, David Landgren via RT wrote: Show quoted text
> One would just have to walk up the call stack and see if there's an > END block there.
I didn't only mean an explicit END{} block. I intended to include all cases where "it's the end of the program, therefore I don't care what my cwd is after this operation." For example: if we're in an object DESTROY call which is implicitly called at end-of-program, silently ignoring the problem may be okay, but if we're in a DESTROY implicitly called at the end of a lexical scope, generating an error is more appropriate. (My real preference is to not treat this as an error condition in any cases, however.) Show quoted text
> What makes me hesitate is symlinks. If you're in a child directory that > is symlinked in from elsewhere, asking to rmtree() the parent might not > delete what you thought it would, or worse, it might delete what you > thought it wouldn't. I guess it depends on whether the fact that > rmtree() will delete a symlink rather than trying to read it is what you > expected.
I think I'm confused. If you expect rmtree() to behave differently than it does behave, how does it matter what your cwd happens to be? It seems to me, you're describing 2 separate problems: 1. people may not have proper expectations w.r.t rmtree and symlinks. 2. rmtree considers it an error if your cwd is inside the tree you're rm'ing. However, I don't think that detecting case 2 is really enough to prevent case 1 from causing problems. Show quoted text
> My personal opinion, not that it amounts to much, is a feeling that it > is wrong to try and delete your own directory out from under you. > Where are you afterwards?
I have my own opinions, though they're also not necessarily worth much. If the program is about to end, then it mostly doesn't matter what your cwd is afterwards. (It does matter if not-run-yet END code needs a working directory.) That's the source of my compromise above for ignoring the error at least in the "end of the world" scenario. However: In general, I don't chdir(). I treat the current working directory as a global variable which I have no control over. I consider it suspicious if anyone calls chdir(), and I prefer to always use full paths whenever possible. It's never truly safe to assume your cwd is valid or unchanged, after calling someone else's code. Perl doesn't (and can't) provide a "local"- like way to preserve cwd: cases such as the one we're describing make it impossible. Not only that: at any time, any process on the machine can remove your current working directory or move it around. It's _never_ truly safe to assume that if you chdir() to a directory, it will be there at the time your next operation runs (except possibly on systems with OS-enforced file locking). Therefore, I say: give the programmer enough rope to hang themselves. It's easy to recover from "I just deleted my cwd:" do a chdir() after the call to rmtree() instead of before the call to rmtree(). If you program defensively and always use full paths, then you're insulated from problems with your cwd disappearing or changing without your knowledge. (In my current case, I was using chdir, and I got bitten by it: more evidence for my preference. Unfortunately, I don't think I can avoid this particular use of chdir, but I also don't think it would be a Real problem if File::Temp rmtree'd my cwd at end-of-perl time.) Show quoted text
> I think I would rather add a flag 'updir' => 1 which would instruct > rmtree to chdir up out out the directory tree it was being asked to > delete. If you want to do something semantically ambiguous I think you > should say so.
I'm not sure this would be very useful. If you know that what you're doing is going to cause this problem, then you can already preempt it by using a chdir() prior to your rmtree() call. That's how I solved my problem. I guess my main point is: Yes, I put myself in the position of removing my own cwd, and yes it's easy to prevent or repair this without changing rmtree() at all. I just think it shouldn't have been a problem in the first place. I'm fine if this is considered "not a bug." In my initial RT report, I mainly just wanted to point out that it's not as uncommon as it may have seemed at the time. Thanks! Alan Ferrency
Download (untitled) / with headers
text/plain 604b
On Thu Feb 07 16:10:43 2008, MSCHILLI wrote: Show quoted text
> I had a similar issue, looks like there's two possible solutions if > someone does something like this: > > mkpath("/tmp/foo/bar"); > chdir("/tmp/foo/bar"); > rmtree("/tmp/foo");
The more I think about it, the more I like this patch. It stops the program from dying (since that is a last-ditch attempt to prevent a race condition). Adding a conditional to prevent innocuous code from triggering what should be "can't-happen" code is a good idea. Thanks for the patch, I'll probably go with a substr, but the concept is sound. Thanks, David
Download (untitled) / with headers
text/plain 370b
The module now gracefully complains about removing a directory tree if the cwd is within the tree, rather than aborting. This fixed in 2.05, but that release will fail during testing if Test::Output is not installed. Otherwise you should install 2.06, which contains a fix for another bug that Gisle Aas encountered last night. Thanks for your feedback, David Landgren


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.