Bug #83228 for File-Temp: Failures on VMS without elevated privileges

Fri Feb 08 16:31:38 2013 dagolden [...] cpan.org - Ticket created

CC:	Craig Berry <craig.a.berry [...] gmail.com>
Subject:	Failures on VMS without elevated privileges
Date:	Fri, 8 Feb 2013 16:30:58 -0500
To:	bug-file-temp <bug-file-temp [...] rt.cpan.org>
From:	David Golden <dagolden [...] cpan.org>

Forwarding this but report from Craig so we get a ticket for it. Show quoted text

---------- Forwarded message ---------- From: Craig A. Berry Date: Fri, Feb 8, 2013 at 3:43 PM I do see one failure with it in blead and it turns out to be because I was running with elevated privileges when I tested before. Oops. Without privileges, tempfile.t fails like: ok 13 - Close temp file # TEMPDIR: MDA0:[CRAIG.SCRATCH.lZ4Q3_bDDe] ok 14 - Temp directory in temp dir # End of tests. Execute END blocks in directory MDA0:[CRAIG.SCRATCH.lZ4Q3_bDDe] cannot stat initial working directory for MDA0:[000000]: not owner at /perl_root/lib/File/Temp.pm line 929. ok 15 - Dir D0:[CRAIG.BLEAD.CPAN.FILE-TEMP.tmpdiraSxcUC] should not be present not ok 16 - Dir MDA0:[CRAIG.SCRATCH.lZ4Q3_bDDe] should not be present # Failed test 'Dir MDA0:[CRAIG.SCRATCH.lZ4Q3_bDDe] should not be present' # at t/tempfile.t line 30. The trouble starts with the "cannot stat" message which is the following code starting at line 924 in Temp.pm: if (defined $cwd_to_remove) { # We do need to clean up the current directory, and everything # else is done, so get out of there and remove it. my $root = File::Spec->rootdir; chdir $root or die "cannot chdir to $root: $!"; eval { rmtree($cwd_to_remove, $DEBUG, 0); }; warn $@ if ($@ && $^W); } I may be missing something and haven't tried to understand what's going on from the ground up, but it seems a little scary to me to chdir to the root of the filesystem (or on Windows and VMS the root of the volume) and start deleting directories. In fact rmtree is saying just how crazy that is by telling us we're not the owner of that root directory. I would think you would want to do something like this instead: chdir $cwd_to_remove; chdir File::Spec->updir; rmtree $cwd_to_remove ....

Fri Feb 08 16:35:15 2013 TJENNESS [...] cpan.org - Correspondence added

Show quoted text

> The trouble starts with the "cannot stat" message which is the > following code starting at line 924 in Temp.pm: > > if (defined $cwd_to_remove) { > # We do need to clean up the current directory, and everything > # else is done, so get out of there and remove it. > my $root = File::Spec->rootdir; > chdir $root or die "cannot chdir to $root: $!"; > eval { rmtree($cwd_to_remove, $DEBUG, 0); }; > warn $@ if ($@ && $^W); > } >

This came in with the fix to #45246

Fri Feb 08 16:35:21 2013 The RT System itself - Status changed from 'new' to 'open'

Fri Feb 08 17:45:04 2013 dagolden [...] cpan.org - Requestor craig.a.berry@gmail.com added

Fri Feb 08 17:45:28 2013 dagolden [...] cpan.org - Requestor DAGOLDEN deleted

Fri Feb 08 17:45:54 2013 dagolden [...] cpan.org - Severity Important added

Wed Feb 13 21:27:16 2013 dagolden [...] cpan.org - Correspondence added

Show quoted text

> I would think you would want to do something like this > instead: > > chdir $cwd_to_remove; > chdir File::Spec->updir; > rmtree $cwd_to_remove ....

Could you please verify if that works on VMS? If so, it seems more or less sensible to me. More generally, I note that the test for being the directory isn't very robust -- it checks if cwd is equal to the directory to remove, which will fail if cwd is a subdirectory of the directory to remove. I wonder if a better algorithm is "try it; if fails, chdir ".." and try again; stop if we fail at rootdir" David

Thu Feb 14 17:04:05 2013 craig.a.berry [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #83228] Failures on VMS without elevated privileges
Date:	Thu, 14 Feb 2013 16:03:54 -0600
To:	bug-File-Temp [...] rt.cpan.org
From:	"Craig A. Berry" <craig.a.berry [...] gmail.com>

On Wed, Feb 13, 2013 at 8:27 PM, David Golden via RT <bug-File-Temp@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=83228 > >

>> I would think you would want to do something like this >> instead: >> >> chdir $cwd_to_remove; >> chdir File::Spec->updir; >> rmtree $cwd_to_remove ....

> > Could you please verify if that works on VMS? If so, it seems more or > less sensible to me.

Yes, it does. The following gets the test passing: --- Temp.pm;-0 2013-02-07 22:27:21 -0600 +++ Temp.pm 2013-02-14 15:03:45 -0600 @@ -924,8 +924,9 @@ sub _can_do_level { if (defined $cwd_to_remove) { # We do need to clean up the current directory, and everything # else is done, so get out of there and remove it. - my $root = File::Spec->rootdir; - chdir $root or die "cannot chdir to $root: $!"; + chdir $cwd_to_remove or die "cannot chdir to $cwd_to_remove: $!"; + my $updir = File::Spec->updir; + chdir $updir or die "cannot chdir to $updir: $!"; eval { rmtree($cwd_to_remove, $DEBUG, 0); }; warn $@ if ($@ && $^W); } [end] Show quoted text

> > More generally, I note that the test for being the directory isn't very > robust -- it checks if cwd is equal to the directory to remove, which > will fail if cwd is a subdirectory of the directory to remove.

Yes, as far as I can tell, the directories will come in whatever order tempdir() was called, so if there are two paths in the same tree, nothing guarantees the child will come first in the array and get deleted first. Show quoted text

> I wonder if a better algorithm is "try it; if fails, chdir ".." and try > again; stop if we fail at rootdir"

I can't think of a good reason to ever go more than one level up from the directory you are trying to delete and it seems dangerous to me to do so. It *should* be safe because we're always sending an absolute path to rmtree, but why risk putting ourselves in a place in the filesystem above where we need to be? If we can't delete /foo/bar/baz when the cwd is /foo/bar, it seems unlikely we'd have a better chance from /foo or /.

Thu Feb 14 17:45:09 2013 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Thu Feb 14 17:45:33 2013 dagolden [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #83228] Failures on VMS without elevated privileges
Date:	Thu, 14 Feb 2013 17:44:50 -0500
To:	bug-File-Temp [...] rt.cpan.org
From:	David Golden <dagolden [...] cpan.org>

Great! I've pushed that patch to the repo. David On Thu, Feb 14, 2013 at 5:04 PM, Craig A. Berry 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=83228 > > > On Wed, Feb 13, 2013 at 8:27 PM, David Golden via RT > <bug-File-Temp@rt.cpan.org> wrote:

>> <URL: https://rt.cpan.org/Ticket/Display.html?id=83228 > >>

>>> I would think you would want to do something like this >>> instead: >>> >>> chdir $cwd_to_remove; >>> chdir File::Spec->updir; >>> rmtree $cwd_to_remove ....

>> >> Could you please verify if that works on VMS? If so, it seems more or >> less sensible to me.

> > Yes, it does. The following gets the test passing: > > --- Temp.pm;-0 2013-02-07 22:27:21 -0600 > +++ Temp.pm 2013-02-14 15:03:45 -0600 > @@ -924,8 +924,9 @@ sub _can_do_level { > if (defined $cwd_to_remove) { > # We do need to clean up the current directory, and everything > # else is done, so get out of there and remove it. > - my $root = File::Spec->rootdir; > - chdir $root or die "cannot chdir to $root: $!"; > + chdir $cwd_to_remove or die "cannot chdir to $cwd_to_remove: $!"; > + my $updir = File::Spec->updir; > + chdir $updir or die "cannot chdir to $updir: $!"; > eval { rmtree($cwd_to_remove, $DEBUG, 0); }; > warn $@ if ($@ && $^W); > } > [end] >

>> >> More generally, I note that the test for being the directory isn't very >> robust -- it checks if cwd is equal to the directory to remove, which >> will fail if cwd is a subdirectory of the directory to remove.

> > Yes, as far as I can tell, the directories will come in whatever order > tempdir() was called, so if there are two paths in the same tree, > nothing guarantees the child will come first in the array and get > deleted first. >

>> I wonder if a better algorithm is "try it; if fails, chdir ".." and try >> again; stop if we fail at rootdir"

> > I can't think of a good reason to ever go more than one level up from > the directory you are trying to delete and it seems dangerous to me to > do so. It *should* be safe because we're always sending an absolute > path to rmtree, but why risk putting ourselves in a place in the > filesystem above where we need to be? If we can't delete /foo/bar/baz > when the cwd is /foo/bar, it seems unlikely we'd have a better chance > from /foo or /. >

-- David Golden <dagolden@cpan.org> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg

Thu Feb 14 17:45:34 2013 The RT System itself - Status changed from 'patched' to 'open'

Thu Feb 14 17:51:27 2013 dagolden [...] cpan.org - Status changed from 'open' to 'patched'

Sat Apr 02 07:14:22 2016 dagolden [...] cpan.org - Status changed from 'patched' to 'resolved'