[PATCH 0/2] posix: fix fnmatch() inconsistency BZ#30483

[PATCH 0/2] posix: fix fnmatch() inconsistency BZ#30483

[Carlo Marcelo Arenas Belón]

Always verify that the class name provided as part of the pattern
is a valid one before returning a result, so fnmatch() wouldn't
mistakenly return 0 if there was a match but the rest of the bracket
expression is not valid.

Note that for most other cases there were already hard failures and
class name that exceeds the maximum was one of those, so the lack of
hard failures might had been caused by the original copy and
adaptation of the code that matches the first expression.

First patch fixes an even older bug and that is partially covered
in the proposed fix for the issue that is included with the second.

I didn't include a regression test to try to keep the change minimal.

I did FSF copyright assignment for GNU grep before, if that is relevant
for this series.

Tested on x86_64-pc-linux-gnu

Carlo Marcelo Arenas Belón (2):
  fnmatch: allow character class names with 'z'
  posix: correctly detect invalid classes after match in fnmatch()

 posix/fnmatch_loop.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH 1/2] fnmatch: allow character class names with 'z'

[Carlo Marcelo Arenas Belón]

Since d8aaef00a7 (Update., 1999-04-26), there is code to consider
character class names that include that character as invalid

* posix/fnmatch_loop.c: correct inequality

Copyright-paperwork-exempt: yes
---
 posix/fnmatch_loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
index 8aeec9816f..4be2e20141 100644
--- a/posix/fnmatch_loop.c
+++ b/posix/fnmatch_loop.c
@@ -283,7 +283,7 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
                             p += 2;
                             break;
                           }
-                        if (c < L_('a') || c >= L_('z'))
+                        if (c < L_('a') || c > L_('z'))
                           {
                             /* This cannot possibly be a character class name.
                                Match it as a normal range.  */
-- 
2.39.2

[PATCH 2/2] posix: correctly detect invalid classes after match in fnmatch()

[Carlo Marcelo Arenas Belón]

Always validate class names when parsing a [:class:] expression

* posix/fnmatch_loop.c (internal_fnmatch/internal_fnwmatch)
remove comment about an specification bug that is no longer relevant
adjust inequality to include 'z' as a valid character in class names
replace soft failure with a hard one when invalid names are used
---
 posix/fnmatch_loop.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
index 4be2e20141..a6b5c69a95 100644
--- a/posix/fnmatch_loop.c
+++ b/posix/fnmatch_loop.c
@@ -866,29 +866,30 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
                   {
                     if (*p == L_('\0'))
                       return FNM_NOMATCH;
-                    /* XXX 1003.2d11 is unclear if this is right.  */
+
                     ++p;
                   }
                 else if (c == L_('[') && *p == L_(':'))
                   {
-                    int c1 = 0;
-                    const CHAR *startp = p;
+                    CHAR str[CHAR_CLASS_MAX_LENGTH + 1];
+                    size_t c1 = 0;
 
                     while (1)
                       {
                         c = *++p;
-                        if (++c1 == CHAR_CLASS_MAX_LENGTH)
-                          return FNM_NOMATCH;
-
-                        if (*p == L_(':') && p[1] == L_(']'))
+                        if (c == L_(':') && p[1] == L_(']'))
                           break;
 
-                        if (c < L_('a') || c >= L_('z'))
-                          {
-                            p = startp - 2;
-                            break;
-                          }
+                        if (c1 == CHAR_CLASS_MAX_LENGTH ||
+                            (c < L_('a') || c > L_('z')))
+                          return FNM_NOMATCH;
+
+                        str[c1++] = c;
                       }
+                    str[c1] = L_('\0');
+                    if (IS_CHAR_CLASS (str) == 0)
+                      return FNM_NOMATCH;
+
                     p += 2;
                   }
                 else if (c == L_('[') && *p == L_('='))
-- 
2.39.2

Re: [PATCH 1/2] fnmatch: allow character class names with 'z'

[Adhemerval Zanella Netto]

On 23/05/23 04:37, Carlo Marcelo Arenas Belón via Libc-alpha wrote:
> Since d8aaef00a7 (Update., 1999-04-26), there is code to consider
> character class names that include that character as invalid
> 
> * posix/fnmatch_loop.c: correct inequality

Thanks for patch.  There is no need for Copyright entry anymore, so you
can skip it on commit message.  Also, current practice is to both add
a bug report if this is a user-visible issue (which seems so) along with
a testcase to avoid any potential regression.  Cold you provide both?

> 
> Copyright-paperwork-exempt: yes
> ---
>  posix/fnmatch_loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
> index 8aeec9816f..4be2e20141 100644
> --- a/posix/fnmatch_loop.c
> +++ b/posix/fnmatch_loop.c
> @@ -283,7 +283,7 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
>                              p += 2;
>                              break;
>                            }
> -                        if (c < L_('a') || c >= L_('z'))
> +                        if (c < L_('a') || c > L_('z'))
>                            {
>                              /* This cannot possibly be a character class name.
>                                 Match it as a normal range.  */

Re: [PATCH 1/2] fnmatch: allow character class names with 'z'

[Carlo Arenas]

On Tue, May 23, 2023 at 11:09 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>. Also, current practice is to both add
> a bug report if this is a user-visible issue (which seems so) along with
> a testcase to avoid any potential regression.  Cold you provide both?

Sure; but I would like to clarify that the bug I was really targeting
has a bugzilla[1] entry already and the fix[2] for it includes "part
2" of a fix for this.

My assumption was that this bug is too old and has no user effect
(unless someone adds a custom class name with 'z' in their name), and
in the 20 years that had gone by, there are only a handful of those.

Either way, I will be adding tests for both bugs in a v2, but wanted
to be sure you would have them split (which I would normally agree
with), or maybe I should have squashed both commits instead.

Carlo

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=30483
[2] https://patchwork.sourceware.org/project/glibc/patch/20230523073732.6956-3-carenas@gmail.com/

Re: [PATCH 1/2] fnmatch: allow character class names with 'z'

[Adhemerval Zanella Netto]

On 23/05/23 18:55, Carlo Arenas wrote:
> On Tue, May 23, 2023 at 11:09 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>> . Also, current practice is to both add
>> a bug report if this is a user-visible issue (which seems so) along with
>> a testcase to avoid any potential regression.  Cold you provide both?
> 
> Sure; but I would like to clarify that the bug I was really targeting
> has a bugzilla[1] entry already and the fix[2] for it includes "part
> 2" of a fix for this.

Thanks, we are now enforcing regression tests on every bug report.  Since
it already have a reproducer, just follow other fnmatch tests (for instance
posix/tst-fnmatch7.c).

> 
> My assumption was that this bug is too old and has no user effect
> (unless someone adds a custom class name with 'z' in their name), and
> in the 20 years that had gone by, there are only a handful of those.
> 
> Either way, I will be adding tests for both bugs in a v2, but wanted
> to be sure you would have them split (which I would normally agree
> with), or maybe I should have squashed both commits instead.

I would say to just squash them on same patch.

> 
> Carlo
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=30483
> [2] https://patchwork.sourceware.org/project/glibc/patch/20230523073732.6956-3-carenas@gmail.com/