Bug 62135

Summary:	apr_fnmatch: inconsistency between description and implementation related to edge cases
Product:	APR	Reporter:	soapand
Component:	APR	Assignee:	Apache Portable Runtime bugs mailinglist <bugs>
Status:	NEW ---
Severity:	major	Keywords:	PatchAvailable
Priority:	P2
Version:	HEAD
Target Milestone:	---
Hardware:	PC
OS:	All
Attachments:	Extra tests for apr_fnmatch function. Replace submitted standalone test with patch to testfnmatch.c

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.