Mailing List Archive

File::Path regression in 5.8.9
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__
Re: File::Path regression in 5.8.9 [ In reply to ]
On Fri, Nov 14, 2008 at 09:16:42AM +0100, Gisle Aas wrote:
> 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.

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
Re: File::Path regression in 5.8.9 [ In reply to ]
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
Re: File::Path regression in 5.8.9 [ In reply to ]
On 2008-11-14, at 08:46:55 +0000, Nicholas Clark wrote:

> On Fri, Nov 14, 2008 at 09:16:42AM +0100, Gisle Aas wrote:
> > 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.
>
> 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.

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

--
enhance, v.:
To tamper with an image, usually to its detriment.
Re: File::Path regression in 5.8.9 [ In reply to ]
On 2008-11-14, at 10:58:09 +0100, Marcus Holland-Moritz wrote:

> 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.

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

--
millihelen, n.:
The amount of beauty required to launch one ship.
Re: File::Path regression in 5.8.9 [ In reply to ]
On Fri, Nov 14, 2008 at 10:55:49AM +0100, Gisle Aas wrote:
> 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.

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
Re: File::Path regression in 5.8.9 [ In reply to ]
On Nov 14, 2008, at 11:15 , Nicholas Clark wrote:

> On Fri, Nov 14, 2008 at 10:55:49AM +0100, Gisle Aas wrote:
>> 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.
>
> True. I considered this, but then we get back to a version with a CVE,
> don't we?

No. perl-5.8.8 shipped with File-Path-1.08 and the CVE-2008-2827 was
introduced in the 2.xx series.

> 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 ?

Yes, it should be with that regard.

--Gisle
Re: File::Path regression in 5.8.9 [ In reply to ]
Marcus Holland-Moritz wrote:
> On 2008-11-14, at 10:58:09 +0100, Marcus Holland-Moritz wrote:
>
>> 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.
>
> 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.

Without looking at the code, that would be a test for a bugfix I added
recently. It will be a simple matter to revert that to the two-arg form.

I'll get this wrapped up tonight.

Thanks for the report,
David
Re: File::Path regression in 5.8.9 [ In reply to ]
On Fri, Nov 14, 2008 at 11:59:22AM +0100, Gisle Aas wrote:
> On Nov 14, 2008, at 11:15 , Nicholas Clark wrote:
> >True. I considered this, but then we get back to a version with a CVE,
> >don't we?
>
> No. perl-5.8.8 shipped with File-Path-1.08 and the CVE-2008-2827 was
> introduced in the 2.xx series.

I believe we're back to code with CVE-2002-0435

> >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 ?
>
> Yes, it should be with that regard.

But we re-add a CVE, so going backwards is not a good option either.

I would be most happy if File::Path 2.08 removed the check code, and
2.09 re-added it, patched and fixed, and 5.8.9 shipped with 2.08

That way, if there are still lingering bugs in the check code, they don't
afflict everyone who is forced to use core releases only, and banned from
upgrading from CPAN.

Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Nov 19, 2008, at 11:59 , Nicholas Clark wrote:

> On Fri, Nov 14, 2008 at 11:59:22AM +0100, Gisle Aas wrote:
>> On Nov 14, 2008, at 11:15 , Nicholas Clark wrote:
>>> True. I considered this, but then we get back to a version with a
>>> CVE,
>>> don't we?
>>
>> No. perl-5.8.8 shipped with File-Path-1.08 and the CVE-2008-2827 was
>> introduced in the 2.xx series.
>
> I believe we're back to code with CVE-2002-0435

No. File::Path has never had the problem described in CVE-2002-0435.
It's a bit confusing that the current File::Path documentation
reference this CVE; but it's mainly to point out that the implementor
thought about this issue when implementing the new chdir based
approach to rmtree.

But, File::Path 1.08 does have the problem described in
CVE-2005-0448. Several vendors (including us at ActiveState) patched
this, but the patch was not portable to VMS so it could not go into
core perl.

And to complete the CVE history of File::Path; v1.07 fixed
CVE-2004-0452 and was distributed first in perl-5.8.7.

Reviewing all these CVEs today I found that File::Path 2.07 still
suffers from CVE-2005-0448. I'm sure you're delighted to learn that :)

>>> 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 ?
>>
>> Yes, it should be with that regard.
>
> But we re-add a CVE, so going backwards is not a good option either.
>
> I would be most happy if File::Path 2.08 removed the check code, and
> 2.09 re-added it, patched and fixed, and 5.8.9 shipped with 2.08

Seems like a good plan to me. But now it looks like 2.08 also ought
to resolve CVE-2005-0448. I'll work out a patch.

--Gisle


>
>
> That way, if there are still lingering bugs in the check code, they
> don't
> afflict everyone who is forced to use core releases only, and banned
> from
> upgrading from CPAN.
>
> Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Wed, Nov 19, 2008 at 04:25:58PM +0100, Gisle Aas wrote:
> On Nov 19, 2008, at 11:59 , Nicholas Clark wrote:
>
> >On Fri, Nov 14, 2008 at 11:59:22AM +0100, Gisle Aas wrote:
> >>On Nov 14, 2008, at 11:15 , Nicholas Clark wrote:
> >>>True. I considered this, but then we get back to a version with a
> >>>CVE,
> >>>don't we?
> >>
> >>No. perl-5.8.8 shipped with File-Path-1.08 and the CVE-2008-2827 was
> >>introduced in the 2.xx series.
> >
> >I believe we're back to code with CVE-2002-0435
>
> No. File::Path has never had the problem described in CVE-2002-0435.
> It's a bit confusing that the current File::Path documentation
> reference this CVE; but it's mainly to point out that the implementor
> thought about this issue when implementing the new chdir based
> approach to rmtree.
>
> But, File::Path 1.08 does have the problem described in
> CVE-2005-0448. Several vendors (including us at ActiveState) patched
> this, but the patch was not portable to VMS so it could not go into
> core perl.
>
> And to complete the CVE history of File::Path; v1.07 fixed
> CVE-2004-0452 and was distributed first in perl-5.8.7.
>
> Reviewing all these CVEs today I found that File::Path 2.07 still
> suffers from CVE-2005-0448. I'm sure you're delighted to learn that :)

Thanks for that correction and clarification.

So, I have it right that the security summary is that:

5.8.8 ships with File::Path 1.08, which is vulnerable to only CVE-2005-0448
File::Path 2.07 is vulnerable to only CVE-2005-0448

> >I would be most happy if File::Path 2.08 removed the check code, and
> >2.09 re-added it, patched and fixed, and 5.8.9 shipped with 2.08

> Seems like a good plan to me. But now it looks like 2.08 also ought
> to resolve CVE-2005-0448. I'll work out a patch.

OK. But my concern is

5.8.9-RC2 is waiting on this
5.8.9 release is waiting on 5.8.9-RC2
git migration is waiting on 5.8.9 release
realistically, integration work for 5.10.1 waits on git migration
Christmas is waiting on 5.10.1. (Or at least Hogmanay)


And I'd really really like to ship the thing and get on with the rest of my
life.

And right now, *if* File::Path 1.08 is equally vulnerable as File::Path 2.07
plus just-a-fix for the temporary directory deletion problem, then the best
way to minimise the risk of 5.8.9-RC3 is to go back to File::Path 1.08

Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Nov 19, 2008, at 16:43 , Nicholas Clark wrote:

> On Wed, Nov 19, 2008 at 04:25:58PM +0100, Gisle Aas wrote:
>> On Nov 19, 2008, at 11:59 , Nicholas Clark wrote:
>>
>>> On Fri, Nov 14, 2008 at 11:59:22AM +0100, Gisle Aas wrote:
>>>> On Nov 14, 2008, at 11:15 , Nicholas Clark wrote:
>>>>> True. I considered this, but then we get back to a version with a
>>>>> CVE,
>>>>> don't we?
>>>>
>>>> No. perl-5.8.8 shipped with File-Path-1.08 and the CVE-2008-2827
>>>> was
>>>> introduced in the 2.xx series.
>>>
>>> I believe we're back to code with CVE-2002-0435
>>
>> No. File::Path has never had the problem described in CVE-2002-0435.
>> It's a bit confusing that the current File::Path documentation
>> reference this CVE; but it's mainly to point out that the implementor
>> thought about this issue when implementing the new chdir based
>> approach to rmtree.
>>
>> But, File::Path 1.08 does have the problem described in
>> CVE-2005-0448. Several vendors (including us at ActiveState) patched
>> this, but the patch was not portable to VMS so it could not go into
>> core perl.
>>
>> And to complete the CVE history of File::Path; v1.07 fixed
>> CVE-2004-0452 and was distributed first in perl-5.8.7.
>>
>> Reviewing all these CVEs today I found that File::Path 2.07 still
>> suffers from CVE-2005-0448. I'm sure you're delighted to learn
>> that :)
>
> Thanks for that correction and clarification.
>
> So, I have it right that the security summary is that:
>
> 5.8.8 ships with File::Path 1.08, which is vulnerable to only
> CVE-2005-0448
> File::Path 2.07 is vulnerable to only CVE-2005-0448

That's right, but it's just a trivial patch to fix the issue in
File::Path 2.07. In File::Path 1.08 there is no easy fix.

It also means that 5.10.0 suffer from both CVE-2005-0448 and
CVE-2008-2827.

Reading the CVE descriptions and following the links there seem to be
much confusion about which issue is CVE-2005-0448 and which is
CVE-2005-0452. Change 23953 claims to fix CVE-2005-0452, but I
actually think it fixed CVE-2005-0448 and that CVE-2005-0452 is the
one we actually still have outstanding. It's really confusing.
Anyway, the attack that's still possible with 2.07 is the one
described by Paul Szabo in <http://bugs.debian.org/286905>. I belive
the following patch (on top of 2.07) fixes it:

diff --git a/Path.pm b/Path.pm
index 128d95b..f38d242 100644
--- a/Path.pm
+++ b/Path.pm
@@ -333,7 +333,7 @@ sub _rmtree {
}
else {
_error($arg, "cannot remove directory", $canon);
- if (!chmod($perm, ($Is_VMS ?
VMS::Filespec::fileify($root) : $root))
+ if ($Force_Writeable && !chmod($perm, ($Is_VMS ?
VMS::Filespec::fileify($root) : $root))
) {
_error($arg, sprintf("cannot restore
permissions to 0%o",$perm), $canon);
}

David, can you tell us what you want to happen? Can you wrap up 2.08
now?

--Gisle


>>> I would be most happy if File::Path 2.08 removed the check code, and
>>> 2.09 re-added it, patched and fixed, and 5.8.9 shipped with 2.08
>
>> Seems like a good plan to me. But now it looks like 2.08 also ought
>> to resolve CVE-2005-0448. I'll work out a patch.
>
> OK. But my concern is
>
> 5.8.9-RC2 is waiting on this
> 5.8.9 release is waiting on 5.8.9-RC2
> git migration is waiting on 5.8.9 release
> realistically, integration work for 5.10.1 waits on git migration
> Christmas is waiting on 5.10.1. (Or at least Hogmanay)
>
>
> And I'd really really like to ship the thing and get on with the
> rest of my
> life.
>
>
> And right now, *if* File::Path 1.08 is equally vulnerable as
> File::Path 2.07
> plus just-a-fix for the temporary directory deletion problem, then
> the best
> way to minimise the risk of 5.8.9-RC3 is to go back to File::Path 1.08
>
> Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Wed, Nov 19, 2008 at 07:09:20PM +0100, Gisle Aas wrote:
> On Nov 19, 2008, at 16:43 , Nicholas Clark wrote:

>> So, I have it right that the security summary is that:
>>
>> 5.8.8 ships with File::Path 1.08, which is vulnerable to only
>> CVE-2005-0448
>> File::Path 2.07 is vulnerable to only CVE-2005-0448
>
> That's right, but it's just a trivial patch to fix the issue in
> File::Path 2.07. In File::Path 1.08 there is no easy fix.
>
> It also means that 5.10.0 suffer from both CVE-2005-0448 and
> CVE-2008-2827.
>
> Reading the CVE descriptions and following the links there seem to be
> much confusion about which issue is CVE-2005-0448 and which is
> CVE-2005-0452. Change 23953 claims to fix CVE-2005-0452, but I actually
> think it fixed CVE-2005-0448 and that CVE-2005-0452 is the one we
> actually still have outstanding. It's really confusing. Anyway, the
> attack that's still possible with 2.07 is the one described by Paul Szabo
> in <http://bugs.debian.org/286905>. I belive the following patch (on top
> of 2.07) fixes it:

FYI, I just reopened the relevant Debian bugs
<http://bugs.debian.org/286905> (allow creation of setuid files)
<http://bugs.debian.org/286922> (removes arbitrary files)
and updated them.

Contrary to the above, I think change 23953 did fix CVE-2004-0452
(note it's 2004 and not 2005!), and CVE-2005-0448 is the one
outstanding.

Otherwise agreed on all counts.

CVE-2005-0448 corresponds to both of the above Debian bugs. I think
the setuid bug is present in both 5.8.8 and 5.10.0 (up to and including
File::Path 2.07 without your patch, which fixes it for me), while the
'remove arbitrary files' one only applies to 5.8.8 (File::Path 1.08).

I agree it's really confusing. Many thanks for the 2.07 patch.
--
Niko Tyni ntyni@debian.org
Re: File::Path regression in 5.8.9 [ In reply to ]
On Wed, Nov 19, 2008 at 07:09:20PM +0100, Gisle Aas wrote:

> diff --git a/Path.pm b/Path.pm
> index 128d95b..f38d242 100644
> --- a/Path.pm
> +++ b/Path.pm
> @@ -333,7 +333,7 @@ sub _rmtree {
> }
> else {
> _error($arg, "cannot remove directory", $canon);
> - if (!chmod($perm, ($Is_VMS ?
> VMS::Filespec::fileify($root) : $root))
> + if ($Force_Writeable && !chmod($perm, ($Is_VMS ?
> VMS::Filespec::fileify($root) : $root))
> ) {
> _error($arg, sprintf("cannot restore
> permissions to 0%o",$perm), $canon);
> }

Applied to blead as change 35008, with a version bump to 2.07_01

Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Fri, Nov 14, 2008 at 10:58:09AM +0100, Marcus Holland-Moritz wrote:

> 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.

I *almost* applied this to blead, before re-reading the comment about VMS.
I then started to remember how the directory part of VMS path syntax works
( [foo.bar] and [foo.bar.baz] IIRC) and thought "erk, that's likely to
break the assumptions"), so applied the appended.

Having waited a while for David's ongoing denial of service attack from
"real life (TM)" to subside, to no avail, I don't want to wait for another
interminate period as we try to work out how to keep VMS happy too.

Nicholas Clark

Change 35009 by nicholas@nicholas-saigo on 2008/12/04 14:09:20

For now, remove the 'cannot remove [dir] when cwd is [dir]' message,
because the existing code will think that /tmp/abc is a subdirectory
of /tmp/aa, and whilst we have a patch for Win32 and *nix, we've not
tested on VMS, which has "interesting" path syntax.

Affected files ...

... //depot/perl/lib/File/Path.pm#64 edit
... //depot/perl/lib/File/Path.t#21 edit

Differences ...

==== //depot/perl/lib/File/Path.pm#64 (text) ====

@@ -166,19 +166,6 @@
for ($arg->{cwd}) { /\A(.*)\Z/; $_ = $1 } # untaint

for my $p (@$paths) {
- # 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;
- }
-
if ($Is_MacOS) {
$p = ":$p" unless $p =~ /:/;
$p .= ":" unless $p =~ /:\z/;
@@ -746,15 +733,6 @@
to restore its permissions to the original state but failed. The
directory may wind up being left behind.

-=item cannot remove [dir] when cwd is [dir]
-
-The current working directory of the program is F</some/path/to/here>
-and you are attempting to remove an ancestor, such as F</some/path>.
-The directory tree is left untouched.
-
-The solution is to C<chdir> out of the child directory to a place
-outside the directory tree to be removed.
-
=item cannot chdir to [parent-dir] from [child-dir]: [errmsg], aborting. (FATAL)

C<remove_tree>, after having deleted everything and restored the permissions

==== //depot/perl/lib/File/Path.t#21 (xtext) ====

@@ -2,7 +2,7 @@

use strict;

-use Test::More tests => 114;
+use Test::More tests => 108;
use Config;

BEGIN {
@@ -138,43 +138,6 @@
rmtree 'solo';
}

-SKIP: {
- # tests for rmtree() of ancestor directory
- my $nr_tests = 6;
- my $cwd = getcwd() or skip "failed to getcwd: $!", $nr_tests;
- my $dir = catdir($cwd, 'remove');
- my $dir2 = catdir($cwd, 'remove', 'this', 'dir');
-
- skip "failed to mkpath '$dir2': $!", $nr_tests
- unless mkpath($dir2, {verbose => 0});
- skip "failed to chdir dir '$dir2': $!", $nr_tests
- unless chdir($dir2);
-
- rmtree($dir, {error => \$error});
- my $nr_err = @$error;
- is($nr_err, 1, "ancestor error");
-
- if ($nr_err) {
- my ($file, $message) = each %{$error->[0]};
- is($file, $dir, "ancestor named");
- my $ortho_dir = $^O eq 'MSWin32' ? File::Path::_slash_lc($dir2) : $dir2;
- $^O eq 'MSWin32' and $message
- =~ s/\A(cannot remove path when cwd is )(.*)\Z/$1 . File::Path::_slash_lc($2)/e;
- is($message, "cannot remove path when cwd is $ortho_dir", "ancestor reason");
- ok(-d $dir2, "child not removed");
- ok(-d $dir, "ancestor not removed");
- }
- else {
- fail( "ancestor 1");
- fail( "ancestor 2");
- fail( "ancestor 3");
- fail( "ancestor 4");
- }
- chdir $cwd;
- rmtree($dir);
- ok(!(-d $dir), "ancestor now removed");
-};
-
my $count = rmtree({error => \$error});
is( $count, 0, 'rmtree of nothing, count of zero' );
is( scalar(@$error), 0, 'no diagnostic captured' );
Re: File::Path regression in 5.8.9 [ In reply to ]
On Thu, Dec 4, 2008 at 8:13 AM, Nicholas Clark <nick@ccl4.org> wrote:
> On Fri, Nov 14, 2008 at 10:58:09AM +0100, Marcus Holland-Moritz wrote:
>
>> 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.
>
> I *almost* applied this to blead, before re-reading the comment about VMS.
> I then started to remember how the directory part of VMS path syntax works
> ( [foo.bar] and [foo.bar.baz] IIRC) and thought "erk, that's likely to
> break the assumptions"), so applied the appended.
>
> Having waited a while for David's ongoing denial of service attack from
> "real life (TM)" to subside, to no avail, I don't want to wait for another
> interminate period as we try to work out how to keep VMS happy too.

Sorry not to have been paying more attention. If I test mhx's patch
against 2.07 on VMS and it all looks peachy, then we have a way
forward?
Re: File::Path regression in 5.8.9 [ In reply to ]
On Thu, Dec 04, 2008 at 09:38:31AM -0600, Craig A. Berry wrote:

> Sorry not to have been paying more attention. If I test mhx's patch

Also my fault for not realising that I should have asked.

> against 2.07 on VMS and it all looks peachy, then we have a way
> forward?

Yes, but do it today please.

(you'll have to reverse change 35009 to try it, and the patch hunk that
bumps $VERSION to 2.07_01 will be rejected, because I already did that as
part of 35008)

Heck, if it works, please feel free to commit a reversal of 35009, and then
his patch to blead, as I'm not going to be online for much longer.

If you don't get a chance to do this, I propose to go with what we have in
perforce currently.

Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Thu, Dec 4, 2008 at 9:44 AM, Nicholas Clark <nick@ccl4.org> wrote:
> On Thu, Dec 04, 2008 at 09:38:31AM -0600, Craig A. Berry wrote:
>
>> Sorry not to have been paying more attention. If I test mhx's patch
>
> Also my fault for not realising that I should have asked.
>
>> against 2.07 on VMS and it all looks peachy, then we have a way
>> forward?
>
> Yes, but do it today please.
>
> (you'll have to reverse change 35009 to try it, and the patch hunk that
> bumps $VERSION to 2.07_01 will be rejected, because I already did that as
> part of 35008)
>
> Heck, if it works, please feel free to commit a reversal of 35009, and then
> his patch to blead, as I'm not going to be online for much longer.

Will do.

> If you don't get a chance to do this, I propose to go with what we have in
> perforce currently.

Sounds reasonable. I'll be offline for a couple hours but will get to
it as soon as I'm back.
Re: File::Path regression in 5.8.9 [ In reply to ]
On Thu, Dec 4, 2008 at 10:11 AM, Craig A. Berry <craig.a.berry@gmail.com> wrote:
> On Thu, Dec 4, 2008 at 9:44 AM, Nicholas Clark <nick@ccl4.org> wrote:
>> On Thu, Dec 04, 2008 at 09:38:31AM -0600, Craig A. Berry wrote:
>>
>>> Sorry not to have been paying more attention. If I test mhx's patch
>>
>> Also my fault for not realising that I should have asked.
>>
>>> against 2.07 on VMS and it all looks peachy, then we have a way
>>> forward?
>>
>> Yes, but do it today please.
>>
>> (you'll have to reverse change 35009 to try it, and the patch hunk that
>> bumps $VERSION to 2.07_01 will be rejected, because I already did that as
>> part of 35008)
>>
>> Heck, if it works, please feel free to commit a reversal of 35009, and then
>> his patch to blead, as I'm not going to be online for much longer.
>
> Will do.

Marcus's change is in as 35012 with the following slight modification:

--- lib/File/Path.t;-1 Thu Dec 4 14:23:12 2008
+++ lib/File/Path.t Thu Dec 4 14:33:18 2008
@@ -567,3 +567,3 @@ SKIP: {
# create and delete directory
- my $px = "$p/$x";
+ my $px = catdir($p, $x);
ok(mkpath($px));
[end of diff]

That plus an unrelated nit patched in 35013 gets Path.t passing on
VMS. I'm starting full builds of blead and 5.8.x with these changes
but that will take me awhile so I thought I'd let the other smokes go
ahead and get started.
Re: File::Path regression in 5.8.9 [ In reply to ]
I'm still in favor of just removing the subdir test (change 35009) as
this part is anyway totally broken with regards to symlinks. Less
code is better than more, especially in a module like this one.

--Gisle
Re: File::Path regression in 5.8.9 [ In reply to ]
On Thu, Dec 4, 2008 at 4:10 PM, Gisle Aas <gisle@activestate.com> wrote:
> I'm still in favor of just removing the subdir test (change 35009) as this
> part is anyway totally broken with regards to symlinks. Less code is better
> than more, especially in a module like this one.

I don't have that strong a preference one way or the other -- I was
just trying to respond to the notion that there was a fix available
that was known to work everywhere but VMS, where the status was
unknown. Now we know; Marcus's patch works as well on VMS as on Unix
or Windows. Whether that's good enough for 5.8.9 is up to Nicholas.

I haven't really hacked my way through the CVE jungle documented
elsewhere in this thread, but on the particular question of "Is
rmtree($dir) trying to remove the parent of $dir?" it seems to me it
comes down to whether some false negatives are acceptable as long as
there are no false positives. In other words, is it acceptable to
fail to detect an attempt to delete the parent in some circumstances
as long as rmtree never bails out erroneously, thinking it is trying
to delete the parent when it's not? And does the current code meet
that standard?

Also, note that symlinks aren't the only potential cause of false
negatives. You have Unix mount points, VMS logical names, Windows
virtual directories, file shares, mapped drives etc., all of which
make determining whether one directory is the parent of another via
string comparison of path names a bit perilous. Is /Users/craig/foo
the parent of /Volumes/HD1/Users/craig/foo/bar? Yep, but the current
code won't detect that. Though I'm not sure whether there is an
efficient, pure-Perl, cross-platform way to do better.
Re: File::Path regression in 5.8.9 [ In reply to ]
On Thu, Dec 04, 2008 at 11:40:33PM -0600, Craig A. Berry wrote:
> On Thu, Dec 4, 2008 at 4:10 PM, Gisle Aas <gisle@activestate.com> wrote:
> > I'm still in favor of just removing the subdir test (change 35009) as this
> > part is anyway totally broken with regards to symlinks. Less code is better
> > than more, especially in a module like this one.
>
> I don't have that strong a preference one way or the other -- I was
> just trying to respond to the notion that there was a fix available
> that was known to work everywhere but VMS, where the status was
> unknown. Now we know; Marcus's patch works as well on VMS as on Unix
> or Windows. Whether that's good enough for 5.8.9 is up to Nicholas.
>
> I haven't really hacked my way through the CVE jungle documented
> elsewhere in this thread, but on the particular question of "Is
> rmtree($dir) trying to remove the parent of $dir?" it seems to me it
> comes down to whether some false negatives are acceptable as long as
> there are no false positives. In other words, is it acceptable to
> fail to detect an attempt to delete the parent in some circumstances
> as long as rmtree never bails out erroneously, thinking it is trying
> to delete the parent when it's not? And does the current code meet
> that standard?

Yes, I was thinking that - can it produce a false positive?

Also, given the convolutions needed in this code to get it right and portable,
it (ultimately) feels like something that ought to be public

Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Fri, Dec 05, 2008 at 02:38:05PM +0000, Nicholas Clark wrote:

> Yes, I was thinking that - can it produce a false positive?

Gah. And again I hit send too soon. I mean to add:

And we have plenty of time to deliberate this, as unfortunately nothing is
going anywhere until rsync and hence smoking resumes.

Nicholas Clark
Re: File::Path regression in 5.8.9 [ In reply to ]
On Dec 4, 2008, at 21:40 , Craig A. Berry wrote:

> On Thu, Dec 4, 2008 at 4:10 PM, Gisle Aas <gisle@activestate.com>
> wrote:
>> I'm still in favor of just removing the subdir test (change 35009)
>> as this
>> part is anyway totally broken with regards to symlinks. Less code
>> is better
>> than more, especially in a module like this one.
>
> I don't have that strong a preference one way or the other -- I was
> just trying to respond to the notion that there was a fix available
> that was known to work everywhere but VMS, where the status was
> unknown. Now we know; Marcus's patch works as well on VMS as on Unix
> or Windows. Whether that's good enough for 5.8.9 is up to Nicholas.
>
> I haven't really hacked my way through the CVE jungle documented
> elsewhere in this thread, but on the particular question of "Is
> rmtree($dir) trying to remove the parent of $dir?" it seems to me it
> comes down to whether some false negatives are acceptable as long as
> there are no false positives. In other words, is it acceptable to
> fail to detect an attempt to delete the parent in some circumstances
> as long as rmtree never bails out erroneously, thinking it is trying
> to delete the parent when it's not? And does the current code meet
> that standard?

Yes, I think so. The current state of File::Path ought to be good
enough for 5.8.9. I'm happy to defer any further tweak suggestions
until after Nick made his release.

--Gisle

> Also, note that symlinks aren't the only potential cause of false
> negatives. You have Unix mount points, VMS logical names, Windows
> virtual directories, file shares, mapped drives etc., all of which
> make determining whether one directory is the parent of another via
> string comparison of path names a bit perilous. Is /Users/craig/foo
> the parent of /Volumes/HD1/Users/craig/foo/bar? Yep, but the current
> code won't detect that. Though I'm not sure whether there is an
> efficient, pure-Perl, cross-platform way to do better.