Re: New apr_stat/apr_dir_read behavior

[Andreas Magnusson <an...@home.se>]

Garrett Rooney wrote:
> On 12/12/05, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> 
> I just documented the use of APR_INCOMPLETE for both apr_stat and
> apr_dir_read, see revision 356615 in the ASF Subversion repository for
> details.  As far as I can tell the incomplete response just means that
> the finfo isn't complete in both cases, so the response (use the valid
> bitmask to check what part's ok) is the correct solution.
> 
> -garrett
> 

Hi all, I've just completed a look into this problem, and this is my 
conclusion:
In APR 0.9.7 (that is delivered with Subversion 1.3.0 on Windows) there 
is a bug in apr_file_open() in that it ignores the flag APR_READCONTROL. 
This is fixed in 1.2.2. This is the thing that causes the whole problem.

But let us take it from the top:
Subversion calls apr_stat(&finfo, path, APR_FINFO_PROT, pool) to see if 
a file is read-only.
apr_stat() calls resolv_ident() which opens the file to get a handle 
which is passed to apr_file_info_get(). It is here the "bug" occurs. In 
0.9.7 the flag APR_READCONTROL isn't used in apr_open_file() which means 
that we cannot call the Win32 GetSecurityInfo() on the handle (done in 
more_finfo()). This means that we call guess_protection_bits() instead, 
which returns something that is quite, but not entirely correct 
(depending on what you were looking for).
In 1.2.2 instead, we manage to call GetSecurityInfo(), and what is 
returned is the rights the auto-group "Everyone" has on the file. Which 
on my machine are none! Why this is done I do not know. But I think that 
is the quest for another discussion, perhaps on the Subversion list.

/Andreas

Re: New apr_stat/apr_dir_read behavior

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Andreas Magnusson wrote:
> 
> First, sorry if I sounded off like I thought it was your (as in APRs) 
> fault of the problem in Subversion. That was not the meaning.

It might well be there is a bug in Subversion.  I do want you to understand
thought that if you are going to hash out changes to APR, it must happen
here, not on the svn dev lists :)

> I also believe that finding a map between user/group/world and ACLs is 
> very difficult, perhaps impossible, so no real blame there!

Yes; this is a non-trivial problem.

> I do *not* think that APR 1.2.2 is in error, but I have to punt the 
> question if the change in apr_open_file() should be proposed for 
> backport to the more seasoned APR developers, as I do not know if the 
> new behavior would need a apr_foo_ex(), or if it would be considered 
> "just a bug fix" (read: what is the expected behavior?).

If Windows is consistent with Unix etc, then it's behavior is 'correct'.
If it is inconsistent, we have a bug.

Bill

Re: New apr_stat/apr_dir_read behavior

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/20/06, Andreas Magnusson <an...@home.se> wrote:
> Garrett Rooney wrote:
> > On 1/20/06, Andreas Magnusson <an...@home.se> wrote:
> >
> > Well, what I was getting at is that since you can reproduce the
> > problem locally, it would be considerably easier for you to find the
> > change that fixed the problem in the more recent branch (svn log and
> > svn blame should be enough to tell you what revision it was introduced
> > in), and given that, it should be pretty simple for you to apply that
> > change to the 0.9.x source tree and see if it fixes your problem.  If
> > it does, the more experienced developers would have a much easier time
> > reviewing the change and potentially committing it to that branch.
> >
> > -garrett
> >
>
> So true,
> in revision 65343, mturk committed the fix I have tried locally. I have
> not run the tests, but by merging that commit to a checkout of 0.9.7
> makes my little test work the same way as 1.2.2 does. Is this
> information enough, or do you want a patch also? Or more info, tests?
> I'm more than happy to help.

That's enough information, I'll try to take a look at this closer this
weekend once I've got a windows machine in front of me to build on.

-garrett

Re: New apr_stat/apr_dir_read behavior

[Andreas Magnusson <an...@home.se>]

Garrett Rooney wrote:
> On 1/20/06, Andreas Magnusson <an...@home.se> wrote:
> 
> Well, what I was getting at is that since you can reproduce the
> problem locally, it would be considerably easier for you to find the
> change that fixed the problem in the more recent branch (svn log and
> svn blame should be enough to tell you what revision it was introduced
> in), and given that, it should be pretty simple for you to apply that
> change to the 0.9.x source tree and see if it fixes your problem.  If
> it does, the more experienced developers would have a much easier time
> reviewing the change and potentially committing it to that branch.
> 
> -garrett
> 

So true,
in revision 65343, mturk committed the fix I have tried locally. I have 
not run the tests, but by merging that commit to a checkout of 0.9.7 
makes my little test work the same way as 1.2.2 does. Is this 
information enough, or do you want a patch also? Or more info, tests? 
I'm more than happy to help.

/Andreas

Re: New apr_stat/apr_dir_read behavior

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/20/06, Andreas Magnusson <an...@home.se> wrote:

> I also believe that finding a map between user/group/world and ACLs is
> very difficult, perhaps impossible, so no real blame there!
> I do *not* think that APR 1.2.2 is in error, but I have to punt the
> question if the change in apr_open_file() should be proposed for
> backport to the more seasoned APR developers, as I do not know if the
> new behavior would need a apr_foo_ex(), or if it would be considered
> "just a bug fix" (read: what is the expected behavior?).

Well, what I was getting at is that since you can reproduce the
problem locally, it would be considerably easier for you to find the
change that fixed the problem in the more recent branch (svn log and
svn blame should be enough to tell you what revision it was introduced
in), and given that, it should be pretty simple for you to apply that
change to the 0.9.x source tree and see if it fixes your problem.  If
it does, the more experienced developers would have a much easier time
reviewing the change and potentially committing it to that branch.

-garrett

Re: New apr_stat/apr_dir_read behavior

[Andreas Magnusson <an...@home.se>]

Garrett Rooney wrote:
> On 1/20/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> 
>> If you would like to see 1.2.3 (or 1.3.0) 'corrected' please offer a patch.
>> If you are asking for a backport to 0.9.8-dev, we would consider that as well
>> as long as there's no change to ABI and no change to the 'expected' behavior
>> of the functions.  Or, we have to add an ugly apr_foo_ex() which corresponds
>> to new 1.2 code.
>>
>> We hope someday soon 0.9 goes away, but with so many 2.0 apache and current
>> svn installed users, it will take some time, yet.
> 
> +1, I'd love to see a backport of this fix, especially if it happens
> in time for the next 0.9.x release.  We (where 'we' == subversion
> developers) are probably going to want to do a release of APR in the
> reasonably near future to push out BDB 4.4 support, since Subversion's
> starting to grow support for it now, and in order for that to be
> releasable we'll need a version of APR that has configure glue that
> will find it (which I already committed to APR, it just hasn't made it
> in to a release).
> 
> -garrett
> 

Sorry that I reply to the "wrong" reply, but I'm reading through gmane, 
and I'm not friend with my news reader at the moment.

First, sorry if I sounded off like I thought it was your (as in APRs) 
fault of the problem in Subversion. That was not the meaning.
I was more trying to set the record straight as to where the supposed 
problem came from (in the code).
I also believe that finding a map between user/group/world and ACLs is 
very difficult, perhaps impossible, so no real blame there!
I do *not* think that APR 1.2.2 is in error, but I have to punt the 
question if the change in apr_open_file() should be proposed for 
backport to the more seasoned APR developers, as I do not know if the 
new behavior would need a apr_foo_ex(), or if it would be considered 
"just a bug fix" (read: what is the expected behavior?).

Regards,
/Andreas

Re: New apr_stat/apr_dir_read behavior

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/20/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:

> If you would like to see 1.2.3 (or 1.3.0) 'corrected' please offer a patch.
> If you are asking for a backport to 0.9.8-dev, we would consider that as well
> as long as there's no change to ABI and no change to the 'expected' behavior
> of the functions.  Or, we have to add an ugly apr_foo_ex() which corresponds
> to new 1.2 code.
>
> We hope someday soon 0.9 goes away, but with so many 2.0 apache and current
> svn installed users, it will take some time, yet.

+1, I'd love to see a backport of this fix, especially if it happens
in time for the next 0.9.x release.  We (where 'we' == subversion
developers) are probably going to want to do a release of APR in the
reasonably near future to push out BDB 4.4 support, since Subversion's
starting to grow support for it now, and in order for that to be
releasable we'll need a version of APR that has configure glue that
will find it (which I already committed to APR, it just hasn't made it
in to a release).

-garrett

Re: New apr_stat/apr_dir_read behavior

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Andreas thanks for a very enlightning analysis...

Andreas Magnusson wrote:
> Why this is done I do not know.

Well, the auto-group Everyone is used to evaluate the unix 'world' ownership
permission.  Sort of crufty emulation, yes, but needed something.

Long term ACL's in a truly generic/crossplatform implementation are needed.

> But I think that 
> is the quest for another discussion, perhaps on the Subversion list.

LOL now why would that be?

Have 1/2 a mind to veto all offlist patches for a while, seeing as there
is no record, no history of how we came to the decisions to change the code.
These are usually the (few and far between) patches that get us in the most
trouble down the road.

Please keep the discussion on an appropriate list.

If you would like to see 1.2.3 (or 1.3.0) 'corrected' please offer a patch.
If you are asking for a backport to 0.9.8-dev, we would consider that as well
as long as there's no change to ABI and no change to the 'expected' behavior
of the functions.  Or, we have to add an ugly apr_foo_ex() which corresponds
to new 1.2 code.

We hope someday soon 0.9 goes away, but with so many 2.0 apache and current
svn installed users, it will take some time, yet.

Bill