[1/2] fnmatch: allow character class names with 'z'
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
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
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. */
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/
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/
@@ -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. */