You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe Jr." <wr...@rowe-clan.net> on 2011/05/19 19:56:23 UTC
Re: Fwd: DO NOT REPLY [Bug 51219] New: apr_fnmatch infinite loop
on pattern "/*/WEB-INF/"
[moving from embargoed to open discussion]
On 5/19/2011 9:53 AM, Joe Orton wrote:
> On Wed, May 18, 2011 at 04:11:19PM -0500, William Rowe wrote:
>> On 5/18/2011 3:55 PM, Joe Orton wrote:
>>>
>>> I am comparing 1.3.9 with the tip of the 1.4.x branch.
>>
>> If you want to fuzz between system fnmatch (linux, bsd, solaris
>> or wherever) and tip of 1.4.x branch, those difference would also
>> be interesting. I spent most of my time observing the deltas
>> between this new implementation and first bsd followed by linux.
>
>>From looking at diffs of 1.4.x vs glibc, I've found:
>
> a) the "/" inside [] thing discussed already
To recap; refer to 2.13.3 bullet 1. within
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_13_03
This behavior is optional, we choose to treat a[x/z]b as two segments, while
glibc appears to treat this as a range. Both are correct by spec, and ours
is more efficient in that we predetermine the end of segment. We do the same
for "\/" within [].
> b) glibc does not match the pattern "\/" against "/" with FNM_PATHNAME
> set, but APR does:
>
> 0000000052585e22 apr_fnmatch(".*\//", ".//", 2) = 0, glibc=1
Do we want to change this behavior? We must still be careful of the
special meaning of '/' but we can force a mismatch by dropping the
logic at line 218-219, which quietly ignores the leading backslash.
Other tests for \/ must remain.
> c) different interpretation of [ or ] within []
>
> 00000001a40dee60 apr_fnmatch("[^[.]", "l", 0) = 0, glibc=1
> 00000003947f62f0 apr_fnmatch("[[.]", ".", 0) = 0, glibc=1
We are correct here, implementations vary... in this case "[."...
looks like the introduction to a collating symbol. Stefen and I
spent some time on this (w.r.t. character classes) and determined
that where [:xxxx:] had broken syntax, we would rewind to the open
bracket and treat the '[' as a literal in the list of characters
to match.
> d) apr does not seem to handle back-slash-escapes inside [] correctly:
>
> 00000009058acad3 apr_fnmatch(".[\-\z]", ".z", 3) = 1, glibc=0
This is now fixed, I believe we can roll 1.4.5 now when folks are
satisfied that all defects are gone.
> e) a bunch of weird (invalid?) range specifications:
I'm omitting your list for another pass through your tests, now that
we have a fix on 1.4 branch (and trunk) to the issue you caught.
Re: Fwd: DO NOT REPLY [Bug 51219] New: apr_fnmatch infinite loop
on pattern "/*/WEB-INF/"
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 5/20/2011 4:29 AM, Joe Orton wrote:
>
> I've done one short run this morning with the 1.4.x tip. In the diff vs
> glibc, I filtered out patterns with ']', '[' and '/' within [], and also
> patterns containing \/. Only two patterns remained with different
> behaviour:
>
> 000000009199a75c apr_fnmatch("*?[.].", "/..", 4) = 0, glibc=1
> 00000000a504a704 apr_fnmatch("*?*[^aa]", "/.", 4) = 0, glibc=1
>
> the flag used is APR_FNM_PERIOD. APR looks correct for the first, I am
> not sure why the second is matching.
One can argue glibc is correct for both, in that the leading slash
was only significant if APR_FNM_PATHNAME was given. I'd have to reread
the three docs that sometimes disagree on these things. But in any case,
I would not consider this a blocker, and glad to read you found no other
concerning patterns!
Re: Fwd: DO NOT REPLY [Bug 51219] New: apr_fnmatch infinite loop on
pattern "/*/WEB-INF/"
Posted by Joe Orton <jo...@redhat.com>.
On Thu, May 19, 2011 at 12:56:23PM -0500, William Rowe wrote:
> [moving from embargoed to open discussion]
> On 5/19/2011 9:53 AM, Joe Orton wrote:
> > On Wed, May 18, 2011 at 04:11:19PM -0500, William Rowe wrote:
> >> On 5/18/2011 3:55 PM, Joe Orton wrote:
> > b) glibc does not match the pattern "\/" against "/" with FNM_PATHNAME
> > set, but APR does:
> >
> > 0000000052585e22 apr_fnmatch(".*\//", ".//", 2) = 0, glibc=1
>
> Do we want to change this behavior? We must still be careful of the
> special meaning of '/' but we can force a mismatch by dropping the
> logic at line 218-219, which quietly ignores the leading backslash.
> Other tests for \/ must remain.
If the spec doesn't disallow the current behaviour I don't see any harm
in keeping it, the pattern is unambiguous.
...
> I'm omitting your list for another pass through your tests, now that
> we have a fix on 1.4 branch (and trunk) to the issue you caught.
I screwed up the long run I set up last night so it did not complete :(
I've done one short run this morning with the 1.4.x tip. In the diff vs
glibc, I filtered out patterns with ']', '[' and '/' within [], and also
patterns containing \/. Only two patterns remained with different
behaviour:
000000009199a75c apr_fnmatch("*?[.].", "/..", 4) = 0, glibc=1
00000000a504a704 apr_fnmatch("*?*[^aa]", "/.", 4) = 0, glibc=1
the flag used is APR_FNM_PERIOD. APR looks correct for the first, I am
not sure why the second is matching.
Regards, Joe
Re: Fwd: DO NOT REPLY [Bug 51219] New: apr_fnmatch infinite loop on
pattern "/*/WEB-INF/"
Posted by Jeff Trawick <tr...@gmail.com>.
On Thu, May 19, 2011 at 2:07 PM, William A. Rowe Jr.
<wr...@rowe-clan.net> wrote:
> On 5/19/2011 12:56 PM, William A. Rowe Jr. wrote:
>> [moving from embargoed to open discussion]
>>
>> On 5/19/2011 9:53 AM, Joe Orton wrote:
>>
>>> b) glibc does not match the pattern "\/" against "/" with FNM_PATHNAME
>>> set, but APR does:
>>>
>>> 0000000052585e22 apr_fnmatch(".*\//", ".//", 2) = 0, glibc=1
>>
>> Do we want to change this behavior? We must still be careful of the
>> special meaning of '/' but we can force a mismatch by dropping the
>> logic at line 218-219, which quietly ignores the leading backslash.
>> Other tests for \/ must remain.
>
> I would argue to keep our behavior, based on typical shell behavior;
>
> $ ls lib\/.svn
>
> tab completion or pressing enter here both work just fine, and this
> should be reflected by our APR_FNM_PATHNAME behavior as well.
BTW, where are we on the 1.4.5-readiness front? Things have been a
bit hectic here and I may have missed something.
Re: Fwd: DO NOT REPLY [Bug 51219] New: apr_fnmatch infinite loop
on pattern "/*/WEB-INF/"
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 5/19/2011 12:56 PM, William A. Rowe Jr. wrote:
> [moving from embargoed to open discussion]
>
> On 5/19/2011 9:53 AM, Joe Orton wrote:
>
>> b) glibc does not match the pattern "\/" against "/" with FNM_PATHNAME
>> set, but APR does:
>>
>> 0000000052585e22 apr_fnmatch(".*\//", ".//", 2) = 0, glibc=1
>
> Do we want to change this behavior? We must still be careful of the
> special meaning of '/' but we can force a mismatch by dropping the
> logic at line 218-219, which quietly ignores the leading backslash.
> Other tests for \/ must remain.
I would argue to keep our behavior, based on typical shell behavior;
$ ls lib\/.svn
tab completion or pressing enter here both work just fine, and this
should be reflected by our APR_FNM_PATHNAME behavior as well.