| Message ID | 20230523073732.6956-2-carenas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | posix: fix fnmatch() inconsistency BZ#30483 | expand |
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/
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. */