Skip Menu |

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

Report information
The Basics
Id: 44924
Status: resolved
Priority: 0/
Queue: File-Temp

People
Owner: Nobody in particular
Requestors: KENTNL [...] cpan.org
leonerd-cpan [...] leonerd.org.uk
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 0.20
Fixed in: (no value)



Subject: Relative 'CLEANUP' doesn't work after chdir()
The not-entirely-unreasonable sequence of use File::Temp qw( tempdir ); my $tempdir = tempdir( "something.XXXXXX", CLEANUP => 1 ); chdir( $tempdir ); .... do stuff doesn't clean up the directory afterwards, presumably because of the relative path no longer being found. I find a workaround is to use Cwd qw( cwd ); my $tempdir = tempdir( cwd() . "/something.XXXXXX", CLEANUP => 1 ); but perhaps this is something File::Temp should do internally? -- Paul Evans
I was just bitten by this and had to spend a few minutes puzzling out why my temp directories weren't being cleaned up. I feel File::Temp should deal with this and turn relative dirs into absolute ones. I can't see any sane reason why you'd want otherwise, or be relying on the current behavior, that's more important than this bug generator. If it's important, add an option to turn off the rel2abs code. Test attached.
Subject: test.plx
Download test.plx
application/octet-stream 305b

Message body not shown because it is not plain text.

I've put a fix for this (and also RT #44924) on github (git://github.com/timj/perl-File-Temp.git) and also fixed up the tempfile.t tests so that they reall test directory removal. The only downside of all this is that we are now pretty much guaranteed to fail when taint mode is enabled because there are now many cases were File::Spec->rel2abs is called and this returns a tainted result (via Cwd::getcwd). Any thoughts? Presumably if untainting getcwd was easy (by verifying that the directory does exist and contains no odd characters) then Cwd would already be doing it.
It is right for the current directory to be tainted since it is something that comes from the environment. You certainly want a warning for code like my $dir = getcwd(); system "echo $dir"; since it has all sorts of string interpolation bugs. However, that does not mean that it is automatically unsafe to use the directory name returned. On Unix-like systems a path can contain almost any character, is returned without comment from getcwd(), and works fine in chdir(). If you are calling perl's chdir() builtin you do not need to worry about whether the path contains funny characters - only that it is the correct path. Therefore I propose that File::Temp should, just before the final chdir() on cleanup, untaint the directory name. Taint checking will still be in place for any other uses of that string. There is the question of whether File::Temp should reject temporary directory names containing funny characters in order to shield user code. But that is a separate issue.
I suggest using fastcwd() instead of cwd(). The latter runs the external 'cwd' program, which could be causing the trouble.
Subject: Re: [rt.cpan.org #44924] Relative 'CLEANUP' doesn't work after chdir()
Date: Fri, 16 Mar 2012 12:55:33 -1000
To: bug-File-Temp [...] rt.cpan.org
From: Tim Jenness <tjenness [...] cpan.org>
Doesn't help because Cwd.xs explicitly turns on taint for fastcwd. This fails: use Cwd; use File::Spec; use Devel::Peek; my $cwd = Cwd::fastcwd(); print "CWD = $cwd\n"; my $path = File::Spec->catfile($cwd, "xxx" ); open (my $fh, ">", $path ); On Fri, Mar 16, 2012 at 4:50 AM, EDAVIS via RT <bug-File-Temp@rt.cpan.org> wrote: Show quoted text
>       Queue: File-Temp >  Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=44924 > > > I suggest using fastcwd() instead of cwd().  The latter runs the external > 'cwd' program, which could be causing the trouble.

Theres some extra fun things you can do, and if you're malicious, presently you can trick File::Temp into deleting the wrong directory.

 

Of course, its mostly a theoretical problem, I doubt you'd see it "in the wild"

 

my $tempdir = tempdir('TEMPLATE.XXXXX', CLEANUP => 1 );

chdir "somewhere/else";

mkdir "somewhere/else/$tempdir";

exit

 

And this cleans up the wrong temporary directory. =)

Looking forward to seeing this fixed.

Considering testing this might be a bit difficult, I did manage to work out how to test for this case with a bit of fork() magic.

 

It still needs to be tested for portability  and skip the tests on platforms it can't work on, but at least on linux this test works.

Here's a link to the tests source in case you wish to review it in your browser:  https://gist.github.com/4414787

 

Basically, it leverages the fact that reap happens at exit(), and creates the tempdir in a fork, prints the name of the created directory over the pipe to the parent process, and then exits, and the parent then sees if cleanup happened or not.

Subject: directory_reap.t
#!/usr/bin/perl -w use warnings 'all'; use strict; use File::Temp qw( tempdir ); use File::Path qw( make_path ); use File::Spec; use IO::Pipe; use Cwd; sub _cwd { File::Spec->rel2abs(Cwd::cwd) } use Test::More; my (@tests) = ( { make_tempdir_in => '/base/', exit_in => '/base/', label => 'no chdir', }, { make_tempdir_in => '/parent/base/', exit_in => '/parent/', label => 'parent dir', }, { make_tempdir_in => '/base/', exit_in => '/sibling/', label => 'sibling dir', }, { make_tempdir_in => '/base/', exit_in => '/base/nephew/', label => 'nephew dir', }, ); for my $test ( @tests ) { for my $file ( run_test( $test ) ) { ok( ! -e $file , '$file should be gone in ' . $test->{label} . ' case'); } } sub _make_test { my ( $config ) = @_ ; my $label = $config->{label}; my $tempdir = tempdir("test.${label}.XXXX", DIR => _cwd , CLEANUP => 1 ); my $outconfig = { make_tempdir_in => File::Spec->catdir( $tempdir , $config->{make_tempdir_in} ), exit_in => File::Spec->catdir( $tempdir , $config->{exit_in} ), }; make_path( $outconfig->{make_tempdir_in} , { verbose => 0 } ); make_path( $outconfig->{exit_in} , { verbose => 0 } ); return $outconfig; } sub child { my ( $test_config , $write_pipe ) = @_; chdir $test_config->{make_tempdir_in}; note sprintf "child in %s\n", _cwd; my $tempdir = tempdir( "THISWILLNOTDIE.XXXXX", CLEANUP => 1 ); note sprintf "child made %s\n", File::Spec->catdir( Cwd::cwd , $tempdir ); $write_pipe->printf( "%s\n", File::Spec->catdir( Cwd::cwd , $tempdir )); chdir $test_config->{exit_in}; note sprintf "child exiting in %s\n" , _cwd; exit; } sub run_test { my ( $config ) = @_; my $real_config = _make_test( $config ); my $pipe = IO::Pipe->new(); my $pid = fork; if( not $pid ){ child( $real_config, $pipe->writer ); exit 255; } my $read = $pipe->reader(); my (@files) = map { chomp $_;$_ } <$read>; waitpid $pid, 0; return @files; } done_testing;
Subject: Re: [rt.cpan.org #44924] Relative 'CLEANUP' doesn't work after chdir()
Date: Sun, 30 Dec 2012 21:21:47 -0700
To: Per Friberg <bug-File-Temp [...] rt.cpan.org>
From: Tim Jenness <tjenness [...] cpan.org>
Does this still fail in the repository version on github? I store the full path of the temp directory in that version. Haven't released it as I'm bogged down in the fact that the patch breaks taint mode. On Sun, Dec 30, 2012 at 1:08 PM, Kent Fredric via RT < bug-File-Temp@rt.cpan.org> wrote: Show quoted text
> Queue: File-Temp > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=44924 > > > Considering testing this might be a bit difficult, I did manage to work > out how > to test for this case with a bit of fork() magic. > > It still needs to be tested for portability and skip the tests on > platforms it > can't work on, but at least on linux this test works. > > Here's a link to the tests source in case you wish to review it in your > browser: https://gist.github.com/4414787 > > Basically, it leverages the fact that reap happens at exit(), and creates > the > tempdir in a fork, prints the name of the created directory over the pipe > to > the parent process, and then exits, and the parent then sees if cleanup > happened or not. > >
On 2012-12-31 17:22:00, TJENNESS wrote:
Show quoted text
> Does this still fail in the repository version on github?
>
> I store the full path of the temp directory in that version. Haven't
> released it as I'm bogged down in the fact that the patch breaks taint mode.

 

Yep. The test as above works and passes 4/4 =) 
 

Attaching some related discussion as a reminder for later: http://www.nntp.perl.org/group/perl.perl5.porters/2012/07/msg189786.html
On reflection, I think the right thing to do is use abs_path on the created directory and detaint the result. Any damage is already done -- the directory has been created (assuming sufficient permissions), so knowing its full name (with symlinks resolved) doesn't have any further risk.
Subsequent commits are safer, with relative pathnames being given to users and the absolute untainted path stored internally only.