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

[Joe Simmons-Talbott <josimmon@redhat.com>]

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?

>  		      __wmemcpy (wc_extended, wcdigits[n], to_level);
>  		      wc_extended[to_level] = __towctrans (L'0' + n, map);
>  		      wc_extended[to_level + 1] = '\0';
> -		      wcdigits_extended[n] = wc_extended;
> +		      digits_extended[n] = wc_extended;
>  #else
>  		      mbdigits[n]
>  			= curctype->values[_NL_CTYPE_INDIGITS0_MB + n].string;
> @@ -1524,14 +1525,13 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  		      size_t mbdigits_len = last_char - mbdigits[n];
>  
>  		      /*  Allocate memory for extended multibyte digit.  */
> -		      char *mb_extended;
> -		      mb_extended = (char *) alloca (mbdigits_len + mblen + 1);
> +		      char *mb_extended = malloc (mbdigits_len + mblen + 1);

Same here?

Thanks,
Joe