Bug 62135 - apr_fnmatch: inconsistency between description and implementation related to edge cases
Summary: apr_fnmatch: inconsistency between description and implementation related to ...
Status: NEW
Alias: None
Product: APR
Classification: Unclassified
Component: APR (show other bugs)
Version: HEAD
Hardware: PC All
: P2 major (vote)
Target Milestone: ---
Assignee: Apache Portable Runtime bugs mailinglist
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2018-02-26 11:58 UTC by soapand
Modified: 2018-04-06 15:43 UTC (History)
0 users



Attachments
Extra tests for apr_fnmatch function. (7.46 KB, text/plain)
2018-02-26 11:58 UTC, soapand
Details
Replace submitted standalone test with patch to testfnmatch.c (1.03 KB, patch)
2018-04-06 15:26 UTC, William A. Rowe Jr.
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description soapand 2018-02-26 11:58:56 UTC
Created attachment 35746 [details]
Extra tests for apr_fnmatch function.

# Problem description

[apr/include/apr_fnmatch.h:103-104](https://github.com/apache/apr/blob/71cf5aa36f5c5138d3bad87c6c1e8124b8df457f/include/apr_fnmatch.h#L103)

> [] never matches

> [^] and [!] always match without consuming a character.

On practice both of this is false. (See the steps to reproduce below.)


# STR

## Steps
~~~
gcc test-apr_fnmatch.cpp -o test-apr_fnmatch -ldl
./test-apr_fnmatch
~~~

## Actual result
~~~
[user@host ~]# gcc test-apr_fnmatch.cpp -o test-apr_fnmatch -ldl
[user@host ~]# ./test-apr_fnmatch
apr_fnmatch("foo[]]bar", "foo]bar", 0) returns 0
apr_fnmatch("foo[!]bar", "foobar", 0) returns 1
apr_fnmatch("foo[^]bar", "foobar", 0) returns 1
apr_fnmatch("foo[!]]bar", "foo]bar", 0) returns 1
apr_fnmatch("foo[^]]bar", "foo]bar", 0) returns 1
apr_fnmatch("foo[!]]bar", "fooXbar", 0) returns 0
apr_fnmatch("foo[^]]bar", "fooXbar", 0) returns 0
done test_fnmatch
~~~

## Expected result
~~~
[user@host ~]# gcc test-apr_fnmatch.cpp -o test-apr_fnmatch -ldl
[user@host ~]# ./test-apr_fnmatch
done test_fnmatch
~~~


# Root cause
[apr/strings/apr_fnmatch.c:93-95](https://github.com/apache/apr/blob/trunk/strings/apr_fnmatch.c#L93)


# Possible solution
Fix the comment in `apr_fnmatch.h` or fix the implementation in `apr_fnmatch.c` so that they correspond to each other.
AND
Add tests like in the STR (maybe with other expected result).
AND
Thoroughly document all this stuff in the "Wildcards and Regular Expressions" section of the documentation: https://httpd.apache.org/docs/trunk/sections.html#wildcards
And add links to this section from other sections of the documentation which are related to wild-card string, e.g. https://httpd.apache.org/docs/trunk/mod/core.html#directory
Comment 1 William A. Rowe Jr. 2018-03-22 02:30:09 UTC
Commenting so that this lands on my radar. Thank you for the report.
Comment 2 William A. Rowe Jr. 2018-03-22 02:30:09 UTC
Commenting so that this lands on my radar. Thank you for the report.
Comment 3 William A. Rowe Jr. 2018-04-06 15:26:59 UTC
Created attachment 35846 [details]
Replace submitted standalone test with patch to testfnmatch.c

(When submitting code please *don't* change whitespace formatting/style,
it is sunk time for the reviewers.)
Comment 4 William A. Rowe Jr. 2018-04-06 15:32:42 UTC
Comment on attachment 35846 [details]
Replace submitted standalone test with patch to testfnmatch.c

Do not commit to apr as-presented, this is submitter's documentation of
behavior. Need to come to concensus about the proper behavior and then
commit the expected results list.
Comment 5 William A. Rowe Jr. 2018-04-06 15:43:40 UTC
Thanks for your analysis! I agree current behavior is inconsistant.

> # Possible solution
> Fix the comment in `apr_fnmatch.h` or fix the implementation in `apr_fnmatch.c` so that they correspond to each other.

Agreed.

> AND
> Add tests like in the STR (maybe with other expected result).

Agreed, have kicked this off with a patch to test/testfnmatch.c following your suggestion/observation. I am tempted to run these same patterns against the OS fnmatch() to examine where our behavior varies from others, as a non-fatal comparison point. I'll address those expectations in another comment.

> AND
> Thoroughly document all this stuff in the "Wildcards and Regular Expressions" section of the documentation: https://httpd.apache.org/docs/trunk/sections.html#wildcards
> And add links to this section from other sections of the documentation which are related to wild-card string, e.g. https://httpd.apache.org/docs/trunk/mod/core.html#directory

Be aware this is the APR project. Patches to httpd project are welcomed by them, as well. Adding further apr_fnmatch() functional documentation is probably not a bad idea either. Note that https://man.openbsd.org/fnmatch.3 is the same implementation, with additional support for posix character classes.