File::Temp / File::Path interaction

Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=479317
reports that File::Temp and File::Path can behave badly if the user  
has chdir'ed into the temporary directory and it's then cleaned up  
either during object destruction or END block. The reason is that  
rmtree aborts if it can not change back to the directory that it  
started from but does not realise that it started from inside the tree  
that it just deleted.

Should I simply put an eval around the rmtree? Or does that sound too  
simplistic?

I noticed this whilst preparing a new release of File::Temp to sync  
with bleadperl.

I think rmtree now just warns rather than aborting.   See:

http://rt.cpan.org/Public/Bug/Display.html?id=31721

On Wed, Nov 12, 2008 at 8:43 PM, Tim Jenness <t.jenness@jach.hawaii.edu> wrote:
> Should I simply put an eval around the rmtree? Or does that sound too
> simplistic?

I would go ahead and do that, even if File::Path is fixed since people
might upgrade File::Temp and not File::Path.

I'd also put a big warning in the documentation.

-- David

I don't think I would recommend that.  File::Path actually tries to  
croak if you try do delete the current directory before it starts  
deleting, so if you just put an eval around it it will just silently  
fail (under the right conditions).

Interestingly enough the test in File::Path that tries to determine if  
you are about to delete the current directory is broken when symlinks  
are involved in the temp area, so at least here on OS X it currently  
does not get into trouble until after it has done it's job.  The same  
seem to be true in the Debian case from the error messages in the  
linked bug report.

The bad code from File/Path.pm is this:

         # need to fixup case and map \ to / on Windows
         my $ortho_root = $^O eq 'MSWin32' ? _slash_lc($p)          :  
$p;
         my $ortho_cwd  = $^O eq 'MSWin32' ? _slash_lc($arg->{cwd}) :  
$arg->{cwd};
         my $ortho_root_length = length($ortho_root);
         $ortho_root_length-- if $^O eq 'VMS'; # don't compare '.'  
with ']'
         if ($ortho_root_length
             && (substr($ortho_root, 0, $ortho_root_length)
              eq substr($ortho_cwd, 0, $ortho_root_length))) {
             local $! = 0;
             _error($arg, "cannot remove path when cwd is $arg- 
>{cwd}", $p);
             next;
         }

In my case (on OS X) $ortho_root ends up as /var/folders/nR/
nRX26y3CEImK5mQjBeGS1U+++TI/-Tmp-/_hOCE0wbWb while $ortho_cwd is /
private/var/folders/nR/nRX26y3CEImK5mQjBeGS1U+++TI/-Tmp-/_hOCE0wbWb/foo.

I think this issue might be fixed by using realpath($ortho_root) for  
the comparison.

Worse is that you can't delete the tree at "/tmp/x" if your current  
directory is "/tmp/xx".  Try:

   use File::Path qw(rmtree mkpath);
   use Cwd qw(cwd);
   my $p = cwd();
   mkpath("x");
   mkpath("xx");
   chdir("xx");
   rmtree("$p/x");

--Gisle

That's my point.  There are possible reasons why temporary
files/directories can't be deleted on various platforms -- I'm most
familiar with Win32, where files open files inside the temporary
directory prevent removal, etc. This can happen due to virus scanners
operating, directory indexing, etc. I even think that I've had
directory removal be hosed because the OS hadn't finished deleting
files yet.  (Hackery: delete files, sleep 2 seconds, delete
directory.)  It's ugly stuff.

Because File::Temp cleanup is 'magic' -- happening behind the scenes
of a program -- for it to be "user-friendly" in my view, it should
make a best-efforts attempt to clean up files/folders (assuming it's
set to do so), but should not have failure to remove them be fatal to
the ongoing execution of the program -- they are, after all,
temporary, and eventually leftover temporary files/directories can be
removed by the OS or the user.

If someone really cares that a temporary directory is actually
removed, or is a fatal error otherwise, they should create the
temporary directory *without* requesting cleanup and then call rmtree
themselves and confirm that it's gone or die -- i.e. it should be
under user control, not part of the magic of File::Temp.

-- David

That's fair enough.  I just think it might be a bad idea to totally  
silence the rmtree() errors that might occur.  Something like this  
might be better:

    eval { rmtree($dir) }; warn $@ if $@ && $^W;

--Gisle

The last item is typically caused by virus scanners too. You'll (used
to) hit it with the core Perl regression tests if you didn't exclude the
build directory from virus scanning: The tests would try to recreate
temp files with the same names as used before and failed with an
ERROR_DELETE_PENDING error because the virus scanner would not keep up
scanning them fast enough. If you ran the tests manually outside the
harness everything would be fine.

Cheers,
-Jan

That seems fine with me. I'll put together a new release.

On Nov 13, 2008, at 19:03 , Gisle Aas wrote:

> Worse is that you can't delete the tree at "/tmp/x" if your current
> directory is "/tmp/xx".  Try:

I really do think this needs to be fixed before 5.8.9.

Here is more proper test that works in 5.8.8, but fails with File-
Path-2.07 as found in 5.8.9-RC1:

--------------------------------------------
use Test::More tests => 6;

use File::Path qw(rmtree mkpath);
use Cwd qw(cwd);

my $p = cwd();
my $x = "x$$";
my $xx = $x . "x";

# setup
ok(mkpath($xx));
ok(chdir($xx));
END {
     ok(chdir($p));
     ok(rmtree($xx));
}

# create and delete directory
my $px = "$p/$x";
ok(mkpath($px));
ok(rmtree($px), "rmtree");     # fails in File-Path-2.07

__END__

Gisle Aas schreef:

>   eval { rmtree($dir) }; warn $@ if $@ && $^W;

    eval {
        rmtree($dir);
        1;
    }
    or do {
        $@ ||= sprintf "Unknown error in %s, line %s",
                       __PACKAGE__, __LINE__;
        warn $@ if $^W;
    }

or any nice less verbose variant of that, like:

    if ( !eval{ rmtree($dir); 1 } ) {
        $@ ||= "Unknown error";
        warn $@ if $^W;
    }

or:

    eval{ rmtree($dir); 1 }
      or $@ ||= "Unknown error"
        and warn $@ if $^W;

Sigh. You're probably right. So.

Dear perl5-porters, if you want 5.8.9 this year, please propose a fix.
You have a regression test:

> Here is more proper test that works in 5.8.8, but fails with File-
> Path-2.07 as found in 5.8.9-RC1:
>
> --------------------------------------------
> use Test::More tests => 6;
>
> use File::Path qw(rmtree mkpath);
> use Cwd qw(cwd);
>
> my $p = cwd();
> my $x = "x$$";
> my $xx = $x . "x";
>
> # setup
> ok(mkpath($xx));
> ok(chdir($xx));
> END {
>     ok(chdir($p));
>     ok(rmtree($xx));
> }
>
> # create and delete directory
> my $px = "$p/$x";
> ok(mkpath($px));
> ok(rmtree($px), "rmtree"); # fails in File-Path-2.07
>
> __END__
>

Patches expected.

Nicholas Clark

On Nov 14, 2008, at 9:46 , Nicholas Clark wrote:

> Dear perl5-porters, if you want 5.8.9 this year, please propose a fix.

My proposed fix is to remove the code that tries to prevent deletion  
of an anchestor directory from starting up.  Patch attached.

I tried to fix the detection code so that it works when symlinks are  
involved but that just ran into problems with limitations in  
realpath() when rx directory permission where missing.  I think it's  
better to just let code removal code try to do its job and fail when  
it runs into the the real trouble even if the error message ends up a  
bit obscure.

Just reverting back to the File-Path version distributed in 5.8.8 also  
seems like a way to get 5.8.9 done.

--Gisle charset=US-ASCII;
format=flowed
Content-Transfer-Encoding: 7bit

Attachment: File-Path-ortho.patch

Potential patch attached. I'm trying to rely on File::Spec
doing cross-platform things right, as I don't know how all
this will behave on VMS. Also, I don't know how exactly
this is expected to behave on platform supporting the
concept of volumes.

The patch adds the test case above to t/Path.t and passes
all tests on Linux and Windoze.

Marcus

BTW, even though File::Path claims to work with 5.005_04,
it won't run the test suite, due to excessive use of 3-arg
open and "open my $foo", which are both unknown to 5.005.

Marcus

True. I considered this, but then we get back to a version with a CVE,
don't we?

I think I like the idea of reverting the detection code for now, and then
repenting in leisure. Er, well, writing it properly in leisure.

Am I right in thinking that if the detection code is reverted, the behaviour
is no different (and hence no worse) than the behaviour of 5.8.8 ?

Nicholas Clark