diff mbox series

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

Message ID 20230523073732.6956-2-carenas@gmail.com
State Changes Requested
Headers
Series posix: fix fnmatch() inconsistency BZ#30483 |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 pending Patch applied

Commit Message

Carlo Arenas May 23, 2023, 7:37 a.m. UTC 
  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(-)

Comments

Adhemerval Zanella May 23, 2023, 6:09 p.m. UTC | #1 
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.  */
Carlo Arenas May 23, 2023, 9:55 p.m. UTC | #2 
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/
Adhemerval Zanella May 26, 2023, 2:25 p.m. UTC | #3 
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/
diff mbox series

Patch

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.  */