[PATCH] Account for grouping in printf width (bug 23432)

[PATCH] Account for grouping in printf width (bug 23432)

[Andreas Schwab]

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.
---
 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);
 
-- 
2.39.1


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Florian Weimer]

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

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Carlos O'Donell]

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.

-- 
Cheers,
Carlos.

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Andreas Schwab]

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, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Florian Weimer]

* 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

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Xi Ruoyao]

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

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Vincent Lefevre]

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.)

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Carlos O'Donell]

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);
>  

-- 
Cheers,
Carlos.

Re: [PATCH] Account for grouping in printf width (bug 23432)

[Carlos O'Donell]

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);
>  

-- 
Cheers,
Carlos.