| Message ID | mvmpmbawxoj.fsf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Account for grouping in printf width (bug 23432) | expand |
* Andreas Schwab via Libc-alpha: > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index 6e9d104524..b46d932a20 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -195,6 +195,7 @@ tests := \ > tst-gets \ > tst-grouping \ > tst-grouping2 \ > + tst-grouping3 \ > tst-long-dbl-fphex \ > tst-memstream-string \ > tst-obprintf \ Missing $(gen-locales) dependency. The change itself seems okay, it seems to align with POSIX. Not sure if this is still possibly to apply during the freeze.
On 1/21/23 06:40, Florian Weimer wrote: > * Andreas Schwab via Libc-alpha: > >> diff --git a/stdio-common/Makefile b/stdio-common/Makefile >> index 6e9d104524..b46d932a20 100644 >> --- a/stdio-common/Makefile >> +++ b/stdio-common/Makefile >> @@ -195,6 +195,7 @@ tests := \ >> tst-gets \ >> tst-grouping \ >> tst-grouping2 \ >> + tst-grouping3 \ >> tst-long-dbl-fphex \ >> tst-memstream-string \ >> tst-obprintf \ > > Missing $(gen-locales) dependency. The change itself seems okay, it > seems to align with POSIX. Not sure if this is still possibly to > apply during the freeze. I'd like to avoid a partial fix like this just before we release 2.27. My preference would be to review this more thoroughly, apply it when 2.28 opens, and then backport when we have confidence the issue is resolved.
On Jan 22 2023, Carlos O'Donell via Libc-alpha wrote: > On 1/21/23 06:40, Florian Weimer wrote: >> * Andreas Schwab via Libc-alpha: >> >>> diff --git a/stdio-common/Makefile b/stdio-common/Makefile >>> index 6e9d104524..b46d932a20 100644 >>> --- a/stdio-common/Makefile >>> +++ b/stdio-common/Makefile >>> @@ -195,6 +195,7 @@ tests := \ >>> tst-gets \ >>> tst-grouping \ >>> tst-grouping2 \ >>> + tst-grouping3 \ >>> tst-long-dbl-fphex \ >>> tst-memstream-string \ >>> tst-obprintf \ >> >> Missing $(gen-locales) dependency. The change itself seems okay, it >> seems to align with POSIX. Not sure if this is still possibly to >> apply during the freeze. > > I'd like to avoid a partial fix like this just before we release 2.27. This is now bug 30068.
* Andreas Schwab via Libc-alpha: > On Jan 22 2023, Carlos O'Donell via Libc-alpha wrote: > >> On 1/21/23 06:40, Florian Weimer wrote: >>> * Andreas Schwab via Libc-alpha: >>> >>>> diff --git a/stdio-common/Makefile b/stdio-common/Makefile >>>> index 6e9d104524..b46d932a20 100644 >>>> --- a/stdio-common/Makefile >>>> +++ b/stdio-common/Makefile >>>> @@ -195,6 +195,7 @@ tests := \ >>>> tst-gets \ >>>> tst-grouping \ >>>> tst-grouping2 \ >>>> + tst-grouping3 \ >>>> tst-long-dbl-fphex \ >>>> tst-memstream-string \ >>>> tst-obprintf \ >>> >>> Missing $(gen-locales) dependency. The change itself seems okay, it >>> seems to align with POSIX. Not sure if this is still possibly to >>> apply during the freeze. >> >> I'd like to avoid a partial fix like this just before we release 2.27. > > This is now bug 30068. I thought this was a fix for bug 23432, i.e. not a regression in 2.37? Thanks, Florian
On Feb 02 2023, Florian Weimer wrote: > * Andreas Schwab via Libc-alpha: > >> On Jan 22 2023, Carlos O'Donell via Libc-alpha wrote: >> >>> On 1/21/23 06:40, Florian Weimer wrote: >>>> * Andreas Schwab via Libc-alpha: >>>> >>>>> diff --git a/stdio-common/Makefile b/stdio-common/Makefile >>>>> index 6e9d104524..b46d932a20 100644 >>>>> --- a/stdio-common/Makefile >>>>> +++ b/stdio-common/Makefile >>>>> @@ -195,6 +195,7 @@ tests := \ >>>>> tst-gets \ >>>>> tst-grouping \ >>>>> tst-grouping2 \ >>>>> + tst-grouping3 \ >>>>> tst-long-dbl-fphex \ >>>>> tst-memstream-string \ >>>>> tst-obprintf \ >>>> >>>> Missing $(gen-locales) dependency. The change itself seems okay, it >>>> seems to align with POSIX. Not sure if this is still possibly to >>>> apply during the freeze. >>> >>> I'd like to avoid a partial fix like this just before we release 2.27. >> >> This is now bug 30068. > > I thought this was a fix for bug 23432, i.e. not a regression in 2.37? Both are regressions.
On Thu, 2023-02-02 at 11:54 +0100, Andreas Schwab via Libc-alpha wrote: > > > > I'd like to avoid a partial fix like this just before we release > > > > 2.27. > > > > > > This is now bug 30068. > > > > I thought this was a fix for bug 23432, i.e. not a regression in > > 2.37? > > Both are regressions. I can confirm the patch fixes the testcase in BZ#30068 as well. Will test if it fixes the MPFR test failure...
On 2023-01-22 18:20:52 -0500, Carlos O'Donell wrote: > On 1/21/23 06:40, Florian Weimer wrote: > > * Andreas Schwab via Libc-alpha: > > > >> diff --git a/stdio-common/Makefile b/stdio-common/Makefile > >> index 6e9d104524..b46d932a20 100644 > >> --- a/stdio-common/Makefile > >> +++ b/stdio-common/Makefile > >> @@ -195,6 +195,7 @@ tests := \ > >> tst-gets \ > >> tst-grouping \ > >> tst-grouping2 \ > >> + tst-grouping3 \ > >> tst-long-dbl-fphex \ > >> tst-memstream-string \ > >> tst-obprintf \ > > > > Missing $(gen-locales) dependency. The change itself seems okay, it > > seems to align with POSIX. Not sure if this is still possibly to > > apply during the freeze. > > I'd like to avoid a partial fix like this just before we release 2.27. 2.37 > My preference would be to review this more thoroughly, apply it when > 2.28 opens, and then backport when we have confidence the issue is ^^^^ 2.38 > resolved. There's actually another bug, which is a regression in glibc 2.37 and a security issue (possible buffer overflow resulting from this new bug, as additional characters are output): https://sourceware.org/bugzilla/show_bug.cgi?id=30068 and according to https://sourceware.org/bugzilla/show_bug.cgi?id=30068#c5 this patch fixes this new bug. (In bug 23432, I was considering only the precision field, while in bug 30068, this is the width field, though these issues may be related in the implementation.)
On 1/19/23 06:50, Andreas Schwab via Libc-alpha wrote: > This is a partial fix for mishandling of grouping when formatting > integers. It properly computes the width in presence of grouping > characteres when the precision is larger than the number of significant > digits. Thanks for working on this. Are you working on a v2 with the $(gen-locales) dependency fix? It would be great to get the fix Bug 30068 in release/2.37/master. > --- > stdio-common/Makefile | 1 + > stdio-common/tst-grouping3.c | 37 +++++++++++++++++++++++++++++ > stdio-common/vfprintf-process-arg.c | 2 +- > 3 files changed, 39 insertions(+), 1 deletion(-) > create mode 100644 stdio-common/tst-grouping3.c > > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index 6e9d104524..b46d932a20 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -195,6 +195,7 @@ tests := \ > tst-gets \ > tst-grouping \ > tst-grouping2 \ > + tst-grouping3 \ > tst-long-dbl-fphex \ > tst-memstream-string \ > tst-obprintf \ > diff --git a/stdio-common/tst-grouping3.c b/stdio-common/tst-grouping3.c > new file mode 100644 > index 0000000000..0031ad4010 > --- /dev/null > +++ b/stdio-common/tst-grouping3.c > @@ -0,0 +1,37 @@ > +/* Test printf with grouping and padding (bug 23432) > + Copyright (C) 2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <locale.h> > +#include <stdio.h> > +#include <support/check.h> > +#include <support/support.h> > + > +static int > +do_test (void) > +{ > + char buf[80]; > + > + xsetlocale (LC_NUMERIC, "de_DE.UTF-8"); > + > + sprintf (buf, "%+-'13.9d", 1234567); > + TEST_COMPARE_STRING (buf, "+001.234.567 "); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c > index 2c651946df..cd3eaf5c0c 100644 > --- a/stdio-common/vfprintf-process-arg.c > +++ b/stdio-common/vfprintf-process-arg.c > @@ -257,7 +257,7 @@ LABEL (unsigned_number): /* Unsigned number of base BASE. */ > width -= 2; > } > > - width -= workend - string + prec; > + width -= number_length + prec; > > Xprintf_buffer_pad (buf, L_('0'), prec); >
On 1/19/23 06:50, Andreas Schwab via Libc-alpha wrote: > This is a partial fix for mishandling of grouping when formatting > integers. It properly computes the width in presence of grouping > characteres when the precision is larger than the number of significant > digits. I have a new version of this patch which I'll repost shortly after my regression testing passes. I would like to make a slight change in the existing code because prec is changed to mean something else completely in the middle of the digit processing. We should introduce a new usigned integer value to carry forward the additional bytes needed for precision and let the compiler optimize that. It is IMO a small enough change that it still fits into the category of making the code cleaner and solving the regression. I say this because we touch a line that makes no sense to me since 'prec' since the start of processing has meant the total precision parsed from the format specifier. I can also see where we go awry in bug 23432, but I'm really nervous to touch that because existing code may expect the existing practice. Changing to match POSIX is going to be an interesting threading of the needle, either developers never care about this use case or POSIX is just being weird in requiring the additional leading zeroes to have grouping characters. > --- > stdio-common/Makefile | 1 + > stdio-common/tst-grouping3.c | 37 +++++++++++++++++++++++++++++ > stdio-common/vfprintf-process-arg.c | 2 +- > 3 files changed, 39 insertions(+), 1 deletion(-) > create mode 100644 stdio-common/tst-grouping3.c > > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index 6e9d104524..b46d932a20 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -195,6 +195,7 @@ tests := \ > tst-gets \ > tst-grouping \ > tst-grouping2 \ > + tst-grouping3 \ > tst-long-dbl-fphex \ > tst-memstream-string \ > tst-obprintf \ > diff --git a/stdio-common/tst-grouping3.c b/stdio-common/tst-grouping3.c > new file mode 100644 > index 0000000000..0031ad4010 > --- /dev/null > +++ b/stdio-common/tst-grouping3.c > @@ -0,0 +1,37 @@ > +/* Test printf with grouping and padding (bug 23432) > + Copyright (C) 2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <locale.h> > +#include <stdio.h> > +#include <support/check.h> > +#include <support/support.h> > + > +static int > +do_test (void) > +{ > + char buf[80]; > + > + xsetlocale (LC_NUMERIC, "de_DE.UTF-8"); > + > + sprintf (buf, "%+-'13.9d", 1234567); > + TEST_COMPARE_STRING (buf, "+001.234.567 "); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c > index 2c651946df..cd3eaf5c0c 100644 > --- a/stdio-common/vfprintf-process-arg.c > +++ b/stdio-common/vfprintf-process-arg.c > @@ -257,7 +257,7 @@ LABEL (unsigned_number): /* Unsigned number of base BASE. */ > width -= 2; > } > > - width -= workend - string + prec; > + width -= number_length + prec; > > Xprintf_buffer_pad (buf, L_('0'), prec); >
diff --git a/stdio-common/Makefile b/stdio-common/Makefile index 6e9d104524..b46d932a20 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -195,6 +195,7 @@ tests := \ tst-gets \ tst-grouping \ tst-grouping2 \ + tst-grouping3 \ tst-long-dbl-fphex \ tst-memstream-string \ tst-obprintf \ diff --git a/stdio-common/tst-grouping3.c b/stdio-common/tst-grouping3.c new file mode 100644 index 0000000000..0031ad4010 --- /dev/null +++ b/stdio-common/tst-grouping3.c @@ -0,0 +1,37 @@ +/* Test printf with grouping and padding (bug 23432) + Copyright (C) 2023 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <locale.h> +#include <stdio.h> +#include <support/check.h> +#include <support/support.h> + +static int +do_test (void) +{ + char buf[80]; + + xsetlocale (LC_NUMERIC, "de_DE.UTF-8"); + + sprintf (buf, "%+-'13.9d", 1234567); + TEST_COMPARE_STRING (buf, "+001.234.567 "); + + return 0; +} + +#include <support/test-driver.c> diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c index 2c651946df..cd3eaf5c0c 100644 --- a/stdio-common/vfprintf-process-arg.c +++ b/stdio-common/vfprintf-process-arg.c @@ -257,7 +257,7 @@ LABEL (unsigned_number): /* Unsigned number of base BASE. */ width -= 2; } - width -= workend - string + prec; + width -= number_length + prec; Xprintf_buffer_pad (buf, L_('0'), prec);