Re: [PATCH] vfscanf-internal: Remove potentially unbounded allocas

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 03/07/23 15:18, Joe Simmons-Talbott wrote:
> On Mon, Jul 03, 2023 at 11:38:29AM -0300, Adhemerval Zanella wrote:
>> From: Joe Simmons-Talbott <josimmon@redhat.com>
>>
>> Some locales define a list of mapping pairs of alternate digits and
>> separators for input digits (to_inpunct).  This require the scanf
>> to create a list of all possible inputs for the optional type
>> modifier 'I'.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>>  stdio-common/Makefile               |  3 ++
>>  stdio-common/tst-scanf-to_inpunct.c | 78 ++++++++++++++++++++++++++++
>>  stdio-common/vfscanf-internal.c     | 40 ++++++++-------
>>  wcsmbs/Makefile                     |  3 ++
>>  wcsmbs/tst-wscanf-to_inpunct.c      | 79 +++++++++++++++++++++++++++++
>>  5 files changed, 186 insertions(+), 17 deletions(-)
>>  create mode 100644 stdio-common/tst-scanf-to_inpunct.c
>>  create mode 100644 wcsmbs/tst-wscanf-to_inpunct.c
>>
>> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
>> index 8871ec7668..f6d9017ff1 100644
>> --- a/stdio-common/Makefile
>> +++ b/stdio-common/Makefile
>> @@ -231,6 +231,7 @@ tests := \
>>    tst-scanf-binary-gnu11 \
>>    tst-scanf-binary-gnu89 \
>>    tst-scanf-round \
>> +  tst-scanf-to_inpunct \
>>    tst-setvbuf1 \
>>    tst-sprintf \
>>    tst-sprintf-errno \
>> @@ -347,6 +348,7 @@ LOCALES := \
>>    de_DE.ISO-8859-1 \
>>    de_DE.UTF-8 \
>>    en_US.ISO-8859-1 \
>> +  fa_IR.UTF-8 \
>>    hi_IN.UTF-8 \
>>    ja_JP.EUC-JP \
>>    ps_AF.UTF-8 \
>> @@ -366,6 +368,7 @@ $(objpfx)tst-swprintf.out: $(gen-locales)
>>  $(objpfx)tst-vfprintf-mbs-prec.out: $(gen-locales)
>>  $(objpfx)tst-vfprintf-width-i18n.out: $(gen-locales)
>>  $(objpfx)tst-grouping3.out: $(gen-locales)
>> +$(objpfx)tst-scanf-to_inpunct.out: $(gen-locales)
>>  endif
>>  
>>  tst-printf-bz18872-ENV = MALLOC_TRACE=$(objpfx)tst-printf-bz18872.mtrace \
>> diff --git a/stdio-common/tst-scanf-to_inpunct.c b/stdio-common/tst-scanf-to_inpunct.c
>> new file mode 100644
>> index 0000000000..32236ac2dc
>> --- /dev/null
>> +++ b/stdio-common/tst-scanf-to_inpunct.c
>> @@ -0,0 +1,78 @@
>> +/* Test scanf for languages with mapping pairs of alternate digits and
>> +   separators.
>> +   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 <array_length.h>
>> +#include <stdio.h>
>> +#include <support/support.h>
>> +#include <support/check.h>
>> +
>> +/* fa_IR defines to_inpunct for numbers.  */
>> +static const struct
>> +{
>> +  int n;
>> +  const char *str;
>> +} inputs[] =
>> +{
>> +  { 1,    "\xdb\xb1" },
>> +  { 2,    "\xdb\xb2" },
>> +  { 3,    "\xdb\xb3" },
>> +  { 4,    "\xdb\xb4" },
>> +  { 5,    "\xdb\xb5" },
>> +  { 6,    "\xdb\xb6" },
>> +  { 7,    "\xdb\xb7" },
>> +  { 8,    "\xdb\xb8" },
>> +  { 9,    "\xdb\xb9" },
>> +  { 10,   "\xdb\xb1\xdb\xb0" },
>> +  { 11,   "\xdb\xb1\xdb\xb1" },
>> +  { 12,   "\xdb\xb1\xdb\xb2" },
>> +  { 13,   "\xdb\xb1\xdb\xb3" },
>> +  { 14,   "\xdb\xb1\xdb\xb4" },
>> +  { 15,   "\xdb\xb1\xdb\xb5" },
>> +  { 16,   "\xdb\xb1\xdb\xb6" },
>> +  { 17,   "\xdb\xb1\xdb\xb7" },
>> +  { 18,   "\xdb\xb1\xdb\xb8" },
>> +  { 19,   "\xdb\xb1\xdb\xb9" },
>> +  { 20,   "\xdb\xb2\xdb\xb0" },
>> +  { 30,   "\xdb\xb3\xdb\xb0" },
>> +  { 40,   "\xdb\xb4\xdb\xb0" },
>> +  { 50,   "\xdb\xb5\xdb\xb0" },
>> +  { 60,   "\xdb\xb6\xdb\xb0" },
>> +  { 70,   "\xdb\xb7\xdb\xb0" },
>> +  { 80,   "\xdb\xb8\xdb\xb0" },
>> +  { 90,   "\xdb\xb9\xdb\xb0" },
>> +  { 100,  "\xdb\xb1\xdb\xb0\xdb\xb0" },
>> +  { 1000, "\xdb\xb1\xdb\xb0\xdb\xb0\xdb\xb0" },
>> +};
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  xsetlocale (LC_ALL, "fa_IR.UTF-8");
>> +
>> +  for (int i = 0; i < array_length (inputs); i++)
>> +    {
>> +      int n;
>> +      sscanf (inputs[i].str, "%Id", &n);
>> +      TEST_COMPARE (n, inputs[i].n);
>> +    }
>> +
>> +  return 0;
>> +}
>> +
>> +#include <support/test-driver.c>
>> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
>> index bfb9baa21a..ba4b289de6 100644
>> --- a/stdio-common/vfscanf-internal.c
>> +++ b/stdio-common/vfscanf-internal.c
>> @@ -1455,13 +1455,14 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  	      int from_level;
>>  	      int to_level;
>>  	      int level;
>> +	      enum { num_digits_len = 10 };
>>  #ifdef COMPILE_WSCANF
>> -	      const wchar_t *wcdigits[10];
>> -	      const wchar_t *wcdigits_extended[10];
>> +	      const wchar_t *wcdigits[num_digits_len];
>>  #else
>> -	      const char *mbdigits[10];
>> -	      const char *mbdigits_extended[10];
>> +	      const char *mbdigits[num_digits_len];
>>  #endif
>> +	      CHAR_T *digits_extended[num_digits_len] = { NULL };
>> +
>>  	      /*  "to_inpunct" is a map from ASCII digits to their
>>  		  equivalent in locale. This is defined for locales
>>  		  which use an extra digits set.  */
>> @@ -1482,18 +1483,18 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  		  /*  Adding new level for extra digits set in locale file.  */
>>  		  ++to_level;
>>  
>> -		  for (n = 0; n < 10; ++n)
>> +		  for (n = 0; n < num_digits_len; ++n)
>>  		    {
>>  #ifdef COMPILE_WSCANF
>>  		      wcdigits[n] = (const wchar_t *)
>>  			_NL_CURRENT (LC_CTYPE, _NL_CTYPE_INDIGITS0_WC + n);
>>  
>>  		      wchar_t *wc_extended = (wchar_t *)
>> -			alloca ((to_level + 2) * sizeof (wchar_t));
>> +			malloc ((to_level + 2) * sizeof (wchar_t));
> 
> Don't we need to handle the case where malloc might fail here?

Indeed, I will update the patch.