File::Path race condition fixed (new, improved)

Porters,

I have documented all the diagnostics of the module and
normalised them 
somewhat. At the same time I cleaned up the error handling
as it was 
beginning to acquire a distinctive pasta-like flavour.

The first patch is the previous patch of a couple of days
ago, the 
second patch should be applied on top.

I have not yet taken John E. Malmberg's comments into
account.

Thanks,
David

On 8/2/07, David Landgren <davidlandgren.net> wrote:
> I have documented all the diagnostics of the module and
normalised them
> somewhat. At the same time I cleaned up the error
handling as it was
> beginning to acquire a distinctive pasta-like flavour.
>
> The first patch is the previous patch of a couple of
days ago, the
> second patch should be applied on top.

With both patches applied to a recent snapshot of blead on
VMS, I see
one test failure in lib/File/Path.t:

not ok 23 - removed directory (old style 2)
#   Failed test 'removed directory (old style 2)'
#   at [-.lib.file]path.t line 105.
#          got: '0'
#     expected: '1'

The test is running the following code:

$count = rmtree($dir2, 0, 1);
is($count, 1, "removed directory (old style 2)");

The third argument is the safety switch meaning "skip
this file if I
don't have delete access." Unless I'm reading this
wrong, before the
race patch, that meant do C<chmod 0700> and then if I
*still* don't
have delete access, skip the file.  After the race patch, it
now means
don't even do a chmod; just skip it if I don't already have
delete
access.  In the case where the test fails, we could give
ourselves
delete access if we wanted to, but we don't even attempt
it.

On other OSs, mkpath probably creates the directories with
whatever
you need to delete them in the first place so you wouldn't
notice from
the test that the skip behavior is now more conservative. 
But if you
set up a directory where you had the ability to change its
permissions
but didn't have the ability to delete it before changing
them, I think
you'd see the same thing as on VMS.  I guess I should
shuffle off and
try to reproduce that so I know I'm not just making this
up.

I'm not sure if the change was intended, but if we no longer
attempt
to give ourselves delete access to files where it's not on
by default,
I think a lot of things in the wild will break.

Craig Berry wrote:

[...]

> $count = rmtree($dir2, 0, 1);
> is($count, 1, "removed directory (old style
2)");
> 
> The third argument is the safety switch meaning
"skip this file if I
> don't have delete access." Unless I'm reading this
wrong, before the
> race patch, that meant do C<chmod 0700> and then
if I *still* don't
> have delete access, skip the file.  After the race
patch, it now means
> don't even do a chmod; just skip it if I don't already
have delete
> access.  In the case where the test fails, we could
give ourselves
> delete access if we wanted to, but we don't even
attempt it.

oops.

> On other OSs, mkpath probably creates the directories
with whatever
> you need to delete them in the first place so you
wouldn't notice from
> the test that the skip behavior is now more
conservative.  But if you
> set up a directory where you had the ability to change
its permissions
> but didn't have the ability to delete it before
changing them, I think
> you'd see the same thing as on VMS.  I guess I should
shuffle off and
> try to reproduce that so I know I'm not just making
this up.
> 
> I'm not sure if the change was intended, but if we no
longer attempt
> to give ourselves delete access to files where it's not
on by default,
> I think a lot of things in the wild will break.

Definitely not intended and to be honest I'm actually not
surprised. 
Between the 'safe' delete mode, the 'Force_Writable'
platforms and going 
off to the races, it's all a bit too convoluted for my
taste. I know 
I've axed some Mac_OS code that needs to go back in, for
instance.

There is also a distinct lack of symmetry in the code
between getting 
things set up so that deletes can take place and unwinding
settings 
afterwards if things didn't work out.

I'll look at the code paths and work out how to get your
chmod back in 
there.

Thanks,
DAvid

On 8/4/07, David Landgren <davidlandgren.net> wrote:
> Craig Berry wrote:

> > $count = rmtree($dir2, 0, 1);
> > is($count, 1, "removed directory (old style
2)");
> >
> > The third argument is the safety switch meaning
"skip this file if I
> > don't have delete access." Unless I'm reading
this wrong, before the
> > race patch, that meant do C<chmod 0700> and
then if I *still* don't
> > have delete access, skip the file.  After the race
patch, it now means
> > don't even do a chmod; just skip it if I don't
already have delete
> > access.  In the case where the test fails, we
could give ourselves
> > delete access if we wanted to, but we don't even
attempt it.


> I'll look at the code paths and work out how to get
your chmod back in
> there.

In the section of the patch that looks like:

-            if (!chmod($rp | 0700,
-                ($Is_VMS ? VMS::Filespec::fileify($root) :
$root))
-            ) {
-                if (!$arg->) {
+
+            if (!($arg-> or $nperm == $perm or
chmod($nperm, $curdir))) {


The previously unqualified chmod is now getting short
circuited by
what it's getting ORed with.  Reordering the OR like so:

--- lib/File/Path.pm;-1 Fri Aug  3 14:13:40 2007
+++ lib/File/Path.pm    Sat Aug  4 09:45:35 2007
 -734,7
+734,7  sub _rmtree {
            # to recurse in which case we are better than rm
-rf for
            # subtrees with strange permissions

-            if (!($arg-> or $nperm == $perm or
chmod($nperm, $curdir))) {
+            if (!(chmod($nperm, $curdir) or $arg->
or $nperm == $perm)) {
                 _error($arg, "cannot make directory
read+writeable", $canon);
                 $nperm = $perm;
             }
[end of patch]

gets all tests passing on VMS.  But I don't know whether the
existing
order was there for a reason and what implications there
might be from
changing it.

Craig Berry wrote:
> On 8/4/07, David Landgren <davidlandgren.net> wrote:
>> Craig Berry wrote:
> 
>>> $count = rmtree($dir2, 0, 1);
>>> is($count, 1, "removed directory (old
style 2)");
>>>
>>> The third argument is the safety switch meaning
"skip this file if I
>>> don't have delete access." Unless I'm
reading this wrong, before the
>>> race patch, that meant do C<chmod 0700>
and then if I *still* don't
>>> have delete access, skip the file.  After the
race patch, it now means
>>> don't even do a chmod; just skip it if I don't
already have delete
>>> access.  In the case where the test fails, we
could give ourselves
>>> delete access if we wanted to, but we don't
even attempt it.

The trouble is that the original test suite never contained
anything to 
nail down exactly what the documentation meant. It's only
the tests I 
added that caught this.

It could be that the code was always wrong, and no-one ever
encountered 
the condition, or more likely, never stopped to notice that
of all the 
files that wound up not being deleted in a large tree, some
of them 
could have been deleted.

>> I'll look at the code paths and work out how to get
your chmod back in
>> there.
> 
> In the section of the patch that looks like:
> 
> -            if (!chmod($rp | 0700,
> -                ($Is_VMS ?
VMS::Filespec::fileify($root) : $root))
> -            ) {
> -                if (!$arg->) {
> +
> +            if (!($arg-> or $nperm == $perm
or chmod($nperm, $curdir))) {
> 
> 
> The previously unqualified chmod is now getting short
circuited by
> what it's getting ORed with.  Reordering the OR like
so:
> 
> --- lib/File/Path.pm;-1 Fri Aug  3 14:13:40 2007
> +++ lib/File/Path.pm    Sat Aug  4 09:45:35 2007
>  -734,7 +734,7  sub _rmtree {
>             # to recurse in which case we are better
than rm -rf for
>             # subtrees with strange permissions
> 
> -            if (!($arg-> or $nperm == $perm
or chmod($nperm, $curdir))) {
> +            if (!(chmod($nperm, $curdir) or
$arg-> or $nperm == $perm)) {
>                  _error($arg, "cannot make
directory read+writeable", $canon);
>                  $nperm = $perm;
>              }
> [end of patch]
> 
> gets all tests passing on VMS.  But I don't know
whether the existing
> order was there for a reason and what implications
there might be from
> changing it.

Well, to go back to the code as it was before I started
renovations, it 
looked like this:

   chmod($rp | 0700, ($Is_VMS ?
VMS::Filespec::fileify($root) : $root))
     or carp ("Can't make directory $root
read+writeable: $!")
       unless $safe;

Keeping in mind that safe is true by default, then it is
"if not safe, 
then chmod; carp if the chmod fails". If in safe mode,
do nothing. That 
can also be understood as "don't change permissions
from what they are 
currently so that should the process be interrupted, no
filesystem 
objects will be left in a more permissive state". This
also contradicts 
your line of reasoning.

The other thing that puzzles me in the code is that farther
down, there 
are additional calls to chmod(), but these are predicated on
the 
contents of not 'safe', but the $Force_Writeable flag, which
the client 
code has no way of influencing, since its setting is derived
from C<$^O>.

So I am wondering if in fact the test is in fact
highlighting a 
difference between a platform with a coarse-grained
permissions 
architecture (Unix) and a fine-grained (VMS). That is, the
test will 
indeed return a different result on VMS to that of Unix.
There's 
probably a way to so on Windows as well, come to think of
it.

I'm a bit lost here.

The original code:

http://svnweb.mongueurs.net/File-Path/
view/trunk/Path.pm?lang=en&rev=5

as it now stands:

http://svnweb.mongueurs.net/File-Path/view/trunk
/Path.pm?lang=en

David

David Landgren wrote:

> Keeping in mind that safe is true by default, then it
is "if not safe,

Actually, the setting is false by default, but that doesn't
really 
change the debate of what happens when it is true.

> then chmod; carp if the chmod fails". If in safe
mode, do nothing. That 
> can also be understood as "don't change
permissions from what they are 
> currently so that should the process be interrupted, no
filesystem 
> objects will be left in a more permissive state".
This also contradicts 
> your line of reasoning.

David

Craig Berry wrote:
> On 8/2/07, David Landgren <davidlandgren.net> wrote:
>> I have documented all the diagnostics of the module
and normalised them
>> somewhat. At the same time I cleaned up the error
handling as it was
>> beginning to acquire a distinctive pasta-like
flavour.
>>
>> The first patch is the previous patch of a couple
of days ago, the
>> second patch should be applied on top.
> 
> With both patches applied to a recent snapshot of blead
on VMS, I see
> one test failure in lib/File/Path.t:
> 
> not ok 23 - removed directory (old style 2)
> #   Failed test 'removed directory (old style 2)'
> #   at [-.lib.file]path.t line 105.
> #          got: '0'
> #     expected: '1'
> 
> The test is running the following code:
> 
> $count = rmtree($dir2, 0, 1);
> is($count, 1, "removed directory (old style
2)");
> 
> The third argument is the safety switch meaning
"skip this file if I
> don't have delete access." Unless I'm reading this
wrong, before the
> race patch, that meant do C<chmod 0700> and then
if I *still* don't
> have delete access, skip the file.  After the race
patch, it now means
> don't even do a chmod; just skip it if I don't already
have delete
> access.  In the case where the test fails, we could
give ourselves
> delete access if we wanted to, but we don't even
attempt it.

Not quite. It's more like "don't even do a chmod; just
skip it because 
it's not safe"

> On other OSs, mkpath probably creates the directories
with whatever
> you need to delete them in the first place so you
wouldn't notice from
> the test that the skip behavior is now more
conservative.  But if you

By default, yes. But mkpath() can create directories with
whatever 
fantasy permissions you choose.

> set up a directory where you had the ability to change
its permissions
> but didn't have the ability to delete it before
changing them, I think
> you'd see the same thing as on VMS.  I guess I should
shuffle off and
> try to reproduce that so I know I'm not just making
this up.

If you can come up with a test or two that nail the
semantics precisely 
on VMS, I could include them in the test suite, then we'll
be sure of 
never breaking it in the future.

If you have the tuits, you may find inspiration in the 
eg/setup-extra-tests script (available on the CPAN
distribution) as a 
way of setting up a directory tree with a super-user
account, and then 
have the test suite run as an ordinary user (which is the
only way to 
get the test suite to encounter files that it truly cannot
delete).

> I'm not sure if the change was intended, but if we no
longer attempt
> to give ourselves delete access to files where it's not
on by default,
> I think a lot of things in the wild will break.

Actually, the semantics changed before the race patch, and
that was 
wrong. Just prior to the race patch, the code looked like
this:

   if (!chmod($rp | 0700,
     ($Is_VMS ? VMS::Filespec::fileify($root) : $root))
   ) {
     if (!$arg->) {
       # moan
     }
   }

So it did attempt the chmod, when the original code did not.
So the 
check against safe must come first, then the chmod attempt.
If this 
causes different behaviour on Unix and VMS, so be it.

I shall push 2.00_09 onto CPAN, which contains the race
patch, and see 
what happens.

Thanks for listening.
David

On 8/20/07, David Landgren <davidlandgren.net> wrote:
> Craig Berry wrote:
> > On 8/2/07, David Landgren <davidlandgren.net> wrote:
> >> I have documented all the diagnostics of the
module and normalised them
> >> somewhat. At the same time I cleaned up the
error handling as it was
> >> beginning to acquire a distinctive pasta-like
flavour.
> >>
> >> The first patch is the previous patch of a
couple of days ago, the
> >> second patch should be applied on top.
> >
> > With both patches applied to a recent snapshot of
blead on VMS, I see
> > one test failure in lib/File/Path.t:
> >
> > not ok 23 - removed directory (old style 2)
> > #   Failed test 'removed directory (old style 2)'
> > #   at [-.lib.file]path.t line 105.
> > #          got: '0'
> > #     expected: '1'
> >
> > The test is running the following code:
> >
> > $count = rmtree($dir2, 0, 1);
> > is($count, 1, "removed directory (old style
2)");
> >
> > The third argument is the safety switch meaning
"skip this file if I
> > don't have delete access." Unless I'm reading
this wrong, before the
> > race patch, that meant do C<chmod 0700> and
then if I *still* don't
> > have delete access, skip the file.  After the race
patch, it now means
> > don't even do a chmod; just skip it if I don't
already have delete
> > access.  In the case where the test fails, we
could give ourselves
> > delete access if we wanted to, but we don't even
attempt it.
>
> Not quite. It's more like "don't even do a chmod;
just skip it because
> it's not safe"
>
> > On other OSs, mkpath probably creates the
directories with whatever
> > you need to delete them in the first place so you
wouldn't notice from
> > the test that the skip behavior is now more
conservative.  But if you
>
> By default, yes. But mkpath() can create directories
with whatever
> fantasy permissions you choose.
>
> > set up a directory where you had the ability to
change its permissions
> > but didn't have the ability to delete it before
changing them, I think
> > you'd see the same thing as on VMS.  I guess I
should shuffle off and
> > try to reproduce that so I know I'm not just
making this up.
>
> If you can come up with a test or two that nail the
semantics precisely
> on VMS, I could include them in the test suite, then
we'll be sure of
> never breaking it in the future.
>
> If you have the tuits, you may find inspiration in the
> eg/setup-extra-tests script (available on the CPAN
distribution) as a
> way of setting up a directory tree with a super-user
account, and then
> have the test suite run as an ordinary user (which is
the only way to
> get the test suite to encounter files that it truly
cannot delete).
>
> > I'm not sure if the change was intended, but if we
no longer attempt
> > to give ourselves delete access to files where
it's not on by default,
> > I think a lot of things in the wild will break.
>
> Actually, the semantics changed before the race patch,
and that was
> wrong. Just prior to the race patch, the code looked
like this:
>
>    if (!chmod($rp | 0700,
>      ($Is_VMS ? VMS::Filespec::fileify($root) :
$root))
>    ) {
>      if (!$arg->) {
>        # moan
>      }
>    }
>
> So it did attempt the chmod, when the original code did
not. So the
> check against safe must come first, then the chmod
attempt. If this
> causes different behaviour on Unix and VMS, so be it.
>
> I shall push 2.00_09 onto CPAN, which contains the race
patch, and see
> what happens.
>
> Thanks for listening.
> David
>

Thanks for working out all the intersecting/overlapping
changes.
2.00_09 passes all tests for me with a recent blead on VMS. 
I have
not had time to look into any VMS-specific tests, but I'm
not sure any
are needed now that you've restored the original precedence
of the
safety flag.