posix: c_lib_ext: fnmatch: fix escape-oriented regression

[cfriedt]

A regression in 936d027 introduced a subtle bug in the way that escaped expressions were handled.

The regression originated with the assumption that test data (originally adapted from a 3rd-party testsuite) was correct when it was in fact flawed.

Specifically, fnmatch("[[?*\\]", "\\", 0) should fail (FNM_NOMATCH), since the "\" sequence (a single backslash after compilation) escapes the following ']' character, thus leaving the bracket expression incomplete.

As @keith-packard has pointed out, pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_13_01, a bracket expression is only interpreted as a bracket expression, when a proper bracket expression is formed.

Therefore, the pattern is interpreted as the sequence '[', '[', '?', '*', ']' and the call should return FNM_NOMATCH to indicate failure rather than 0 to indicate success.

Added new test cases from #98827 and some commentary for subsequent reviewers.

This change does not completely fix #55186 but is related to it.

The remaining changes are fixed in #97400

[keith-packard]

Specifically, fnmatch("[[?*\\]", "\\", 0) should fail, since the "" sequence (a single backslash after compilation) escapes the following ']' character, thus leaving the bracket expression incomplete. Since the bracket expression (and therefore the whole pattern) is erroneous, the call should return FNM_NOMATCH to indicate failure rather than 0 to indicate success.

The pattern is not erroneous (which would cause the function to return -FNM_NOMATCH). It is a legitimate pattern equivalent to "[[?*]" and not a bracket expression at all. We might want to add a test to verify that the pattern works in this way.

Note that Musl has the same bug, which is why the test was flawed.

[cfriedt]

The pattern is not erroneous (which would cause the function to return -FNM_NOMATCH). It is a legitimate pattern equivalent to "[[?*]" and not a bracket expression at all. We might want to add a test to verify that the pattern works in this way.

Ok - sorry - I was proceeding based on your comment here, where you wrote that ] is escaped, which makes sense to me.
#98827 (comment)

Maybe I was reading-in to your comment too far though, and assumed that if the final ] were escaped, that it would not form a proper bracket expression. To me, it would seem that interpreting an escaped ] without an additional non-escaped ] following it would be more of an opportunistic interpretation to force \] to close the bracket expression rather than being a character to match (assuming a following ] would properly close the bracket expression). I have not found any part of the specification that supports that interpretation though, and the specification also states that escaping an ordinary character outside of a bracket expression is undefined.

In either case, when the pattern is invalid or when it is "[[?*]", fnmatch() should return FNM_NOMATCH; in the former interpretation, to indicate an error of some kind, and in the latter interpretation, the \\ in the input would not be matched.

I can update the PR description and / or commentary as you like, but it would be nice to have a reference to the specification to support it.

I've confirmed that Zephyr's implementation recognizes "[[?*\\]" (with 0 flags) as an unterminated (i.e. invalid) bracket expression.

[cfriedt]

It would seem that even the glibc agrees with the former interpretation (that there is an incomplete bracket expression).

A sample program I wrote to test

#include <fnmatch.h>
#include <stdio.h>

int main()
{
  const char *pat = "[abc\\]";
  const char *in = "a";
  int ret;

  ret = fnmatch(pat, in, 0);
  if (ret == 0) {
    printf("fnmatch(\"%s\", \"%s\", 0) -> success!\n", pat, in);
  } else {
    printf("fnmatch(\"%s\", \"%s\", 0) -> failed! (%d)\n", pat, in, ret);
  }

  return ret;
}

produces the output below.

$ /tmp/blah 
fnmatch("[abc\]", "a", 0) -> failed! (1)

It fails as well with FNM_NOMATCH when pat = "[[?*\\]" and in = "\\".

[keith-packard]

In either case, when the pattern is invalid or when it is "[[?*]", fnmatch() should return FNM_NOMATCH; in the former interpretation, to indicate an error of some kind, and in the latter interpretation, the \\ in the input would not be matched.

I think you've mis-interpreted my comment -- yes, this is an invalid bracket expression, but https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_13_01 says that you only get a bracket expression when the bracket expression is valid, otherwise you interpret the bytes as not being part of a bracket expression at all. An valid expression which does not match the string returns FNM_NOMATCH, an erroneous expression returns some other non-zero value (glibc, picolibc and Zephyr all return -FNM_NOMATCH).

So, the difference is which return value is expected, in this case the expression is valid (although does not contain a bracket expression), it's just that it doesn't match the string, so it should return FNM_NOMATCH.

[cfriedt]

@keith-packard - thanks for clarifying. I've updated the PR description and comment.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[cfriedt]

@keith-packard - just a gentle ping to re-review.

[keith-packard]

This test is incorrect. The first path component, "a", matches *[![:digit:]]* as 'a' is not a digit. The second path component, "b", matches [![:d-d] because 'b' is not [, not : and not any of the range d-d.

[keith-packard]

Please fix the remaining bugs in the implementation and update the tests to conform with the spec.

[keith-packard]

This test is also incorrect, again "a" matches *[![:digit:]]* while "[" matches one of [, : or the range d-d.

I'd strongly prefer that the bugs in the Zephyr implementation be fixed before this series is merged; there's no particular reason to commit known-buggy code, especially as this function doesn't appear to be urgently in need of updates.