[PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ #30647]

[PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ #30647]

[Avinal Kumar]

The scanf family of functions like sscanf and fscanf currently
ignore nan() and nan(n-char-sequence).  This happens because
__vfscanf_internal only checks for 'nan'.

This commit adds support for all valid nan types i.e.  nan, nan()
and nan(n-char-sequence), where n-char-sequence can be
[a-zA-Z0-9_]+, thus fixing the bug 30647.  Any other representation
of NaN should result in conversion error.

New tests are also added to verify the correct parsing of NaN types.

Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>
---
Changes from v1:
I found a corner case where the loop was ending successfully without
respecting the legal exit conditions.  This happened because the loop was
dependent on width size.  Since it is do-while loop, it was not checking
the last character.  So I changed the loop condition to exit only when ')'
is observed.

 stdio-common/Makefile           |  1 +
 stdio-common/tst-scanf-nan.c    | 82 +++++++++++++++++++++++++++++++++
 stdio-common/vfscanf-internal.c | 46 +++++++++++++++++-
 3 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 stdio-common/tst-scanf-nan.c

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 88105b3c1b..a166eb7cf8 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -261,6 +261,7 @@ tests := \
   tst-scanf-binary-gnu89 \
   tst-scanf-bz27650 \
   tst-scanf-intn \
+  tst-scanf-nan \
   tst-scanf-round \
   tst-scanf-to_inpunct \
   tst-setvbuf1 \
diff --git a/stdio-common/tst-scanf-nan.c b/stdio-common/tst-scanf-nan.c
new file mode 100644
index 0000000000..143bce9aef
--- /dev/null
+++ b/stdio-common/tst-scanf-nan.c
@@ -0,0 +1,82 @@
+/* Test scanf formats for nan, nan(), nan(n-char-sequence) types.
+   Copyright The GNU Toolchain Authors.
+   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 <errno.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#include <support/check.h>
+
+#define CHECK_SCANF_RET(OK, STR, FMT, ...)                                    \
+  do                                                                          \
+    {                                                                         \
+      int ret = sscanf (STR, FMT, __VA_ARGS__);                               \
+      TEST_VERIFY (ret == (OK));                                              \
+    }                                                                         \
+  while (0)
+
+/* Valid nan types:
+   1. nan
+   2. nan()
+   3. nan([a-zA-Z0-9_]+)
+   Any other nan format is invalid and should produce a conversion error.
+   The return value denotes the number of valid conversions.  On conversion
+   error the rest of the input is discarded.  */
+static int
+do_test (void)
+{
+  double a, b, c;
+  int d;
+
+  /* All valid inputs.  */
+  CHECK_SCANF_RET (1, "nan", "%lf", &a);
+  CHECK_SCANF_RET (1, "nan()", "%lf", &a);
+  CHECK_SCANF_RET (1, "nan(12345)", "%lf", &a);
+  CHECK_SCANF_RET (2, "nan12", "%lf%d", &a, &d);
+  CHECK_SCANF_RET (2, "nan nan()", "%lf%lf", &a, &b);
+  CHECK_SCANF_RET (2, "nan nan(12345foo)", "%lf%lf", &a, &b);
+  CHECK_SCANF_RET (3, "nan nan() 12.234", "%lf%lf%lf", &a, &b, &c);
+  CHECK_SCANF_RET (4, "nannan()nan(foo)1234", "%lf%lf%lf%d", &a, &b, &c, &d);
+
+  /* Partially valid inputs.  */
+  CHECK_SCANF_RET (1, "nan( )", "%3lf", &a);
+  CHECK_SCANF_RET (1, "nan nan(", "%lf%lf", &a, &b);
+
+  /* Invalid inputs.  */
+
+  /* Dangling parentheses.  */
+  CHECK_SCANF_RET (0, "nan(", "%lf", &a);
+  CHECK_SCANF_RET (0, "nan(123", "%lf", &a);
+  CHECK_SCANF_RET (0, "nan(12345", "%lf%d", &a, &d);
+
+  /* Field width is not sufficient for valid conversion.  */
+  CHECK_SCANF_RET (0, "nan()", "%4lf", &a);
+  CHECK_SCANF_RET (0, "nan(1", "%5lf", &a);
+
+  /* Space is not a valid character.  */
+  CHECK_SCANF_RET (0, "nan( )", "%lf", &a);
+  CHECK_SCANF_RET (0, "nan( )12.34", "%lf%lf", &a, &b);
+  CHECK_SCANF_RET (0, "nan(12 foo)", "%lf", &a);
+
+  /* Period '.' is not a valid character.  */
+  CHECK_SCANF_RET (0, "nan(12.34) nan(FooBar)", "%lf%lf", &a, &b);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
index 1b82deffa7..3e1423d488 100644
--- a/stdio-common/vfscanf-internal.c
+++ b/stdio-common/vfscanf-internal.c
@@ -2028,7 +2028,51 @@ digits_extended_fail:
 	      if (width > 0)
 		--width;
 	      char_buffer_add (&charbuf, c);
-	      /* It is "nan".  */
+	      /* It is at least "nan".  Now we check for nan() and
+	         nan(n-char-sequence).  */
+	      if (width != 0 && inchar () != EOF)
+		{
+		  if (c == L_ ('('))
+		    {
+		      if (width > 0)
+			--width;
+		      char_buffer_add (&charbuf, c);
+		      /* A '(' was observed, check for a closing ')', there
+			 may or may not be a n-char-sequence in between.  We
+			 have to check the longest prefix until there is a
+			 conversion error or closing parenthesis.  */
+		      do
+			{
+			  if (__builtin_expect (width == 0
+						|| inchar () == EOF, 0))
+			    {
+			      /* Conversion error because we ran out of
+				 characters.  */
+			      conv_error ();
+			      break;
+			    }
+			  if (!((c >= L_ ('0') && c <= L_ ('9'))
+				|| (c >= L_ ('A') && c <= L_ ('Z'))
+				|| (c >= L_ ('a') && c <= L_ ('z'))
+				|| c == L_ ('_') || c == L_ (')')))
+			    {
+			      /* Invalid character was observed.  Only valid
+				 characters are [a-zA-Z0-9_] and ')'.  */
+			      conv_error ();
+			      break;
+			    }
+			  if (width > 0)
+			    --width;
+			  char_buffer_add (&charbuf, c);
+			}
+		      while (c != L_ (')'));
+		      /* The loop only exits successfully when ')' is the
+			 last character.  */
+		    }
+		  else
+		    /* It is only 'nan'.  */
+		    ungetc (c, s);
+		}
 	      goto scan_float;
 	    }
 	  else if (TOLOWER (c) == L_('i'))
-- 
2.47.0

Re: [PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ #30647]

[Adhemerval Zanella Netto]

On 18/10/24 08:57, Avinal Kumar wrote:
> The scanf family of functions like sscanf and fscanf currently
> ignore nan() and nan(n-char-sequence).  This happens because
> __vfscanf_internal only checks for 'nan'.
> 
> This commit adds support for all valid nan types i.e.  nan, nan()
> and nan(n-char-sequence), where n-char-sequence can be
> [a-zA-Z0-9_]+, thus fixing the bug 30647.  Any other representation
> of NaN should result in conversion error.
> 
> New tests are also added to verify the correct parsing of NaN types.
> 
> Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>

Looks ok, some comments below.

> ---
> Changes from v1:
> I found a corner case where the loop was ending successfully without
> respecting the legal exit conditions.  This happened because the loop was
> dependent on width size.  Since it is do-while loop, it was not checking
> the last character.  So I changed the loop condition to exit only when ')'
> is observed.
> 
>  stdio-common/Makefile           |  1 +
>  stdio-common/tst-scanf-nan.c    | 82 +++++++++++++++++++++++++++++++++
>  stdio-common/vfscanf-internal.c | 46 +++++++++++++++++-
>  3 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 stdio-common/tst-scanf-nan.c
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 88105b3c1b..a166eb7cf8 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -261,6 +261,7 @@ tests := \
>    tst-scanf-binary-gnu89 \
>    tst-scanf-bz27650 \
>    tst-scanf-intn \
> +  tst-scanf-nan \
>    tst-scanf-round \
>    tst-scanf-to_inpunct \
>    tst-setvbuf1 \
> diff --git a/stdio-common/tst-scanf-nan.c b/stdio-common/tst-scanf-nan.c
> new file mode 100644
> index 0000000000..143bce9aef
> --- /dev/null
> +++ b/stdio-common/tst-scanf-nan.c
> @@ -0,0 +1,82 @@
> +/* Test scanf formats for nan, nan(), nan(n-char-sequence) types.
> +   Copyright The GNU Toolchain Authors.
> +   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 <errno.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +#include <support/check.h>
> +
> +#define CHECK_SCANF_RET(OK, STR, FMT, ...)                                    \
> +  do                                                                          \
> +    {                                                                         \
> +      int ret = sscanf (STR, FMT, __VA_ARGS__);                               \
> +      TEST_VERIFY (ret == (OK));                                              \
> +    }                                                                         \
> +  while (0)
> +
> +/* Valid nan types:
> +   1. nan
> +   2. nan()
> +   3. nan([a-zA-Z0-9_]+)
> +   Any other nan format is invalid and should produce a conversion error.
> +   The return value denotes the number of valid conversions.  On conversion
> +   error the rest of the input is discarded.  */
> +static int
> +do_test (void)
> +{
> +  double a, b, c;

Maybe also check for float and long double as well?

> +  int d;
> +
> +  /* All valid inputs.  */
> +  CHECK_SCANF_RET (1, "nan", "%lf", &a);
> +  CHECK_SCANF_RET (1, "nan()", "%lf", &a);
> +  CHECK_SCANF_RET (1, "nan(12345)", "%lf", &a);
> +  CHECK_SCANF_RET (2, "nan12", "%lf%d", &a, &d);
> +  CHECK_SCANF_RET (2, "nan nan()", "%lf%lf", &a, &b);
> +  CHECK_SCANF_RET (2, "nan nan(12345foo)", "%lf%lf", &a, &b);
> +  CHECK_SCANF_RET (3, "nan nan() 12.234", "%lf%lf%lf", &a, &b, &c);
> +  CHECK_SCANF_RET (4, "nannan()nan(foo)1234", "%lf%lf%lf%d", &a, &b, &c, &d);

Ok.

> +
> +  /* Partially valid inputs.  */
> +  CHECK_SCANF_RET (1, "nan( )", "%3lf", &a);
> +  CHECK_SCANF_RET (1, "nan nan(", "%lf%lf", &a, &b);

Ok.

> +
> +  /* Invalid inputs.  */
> +
> +  /* Dangling parentheses.  */
> +  CHECK_SCANF_RET (0, "nan(", "%lf", &a);
> +  CHECK_SCANF_RET (0, "nan(123", "%lf", &a);
> +  CHECK_SCANF_RET (0, "nan(12345", "%lf%d", &a, &d);
> +
> +  /* Field width is not sufficient for valid conversion.  */
> +  CHECK_SCANF_RET (0, "nan()", "%4lf", &a);
> +  CHECK_SCANF_RET (0, "nan(1", "%5lf", &a);
> +
> +  /* Space is not a valid character.  */
> +  CHECK_SCANF_RET (0, "nan( )", "%lf", &a);
> +  CHECK_SCANF_RET (0, "nan( )12.34", "%lf%lf", &a, &b);
> +  CHECK_SCANF_RET (0, "nan(12 foo)", "%lf", &a);
> +
> +  /* Period '.' is not a valid character.  */
> +  CHECK_SCANF_RET (0, "nan(12.34) nan(FooBar)", "%lf%lf", &a, &b);

Ok.

> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
> index 1b82deffa7..3e1423d488 100644
> --- a/stdio-common/vfscanf-internal.c
> +++ b/stdio-common/vfscanf-internal.c
> @@ -2028,7 +2028,51 @@ digits_extended_fail:
>  	      if (width > 0)
>  		--width;
>  	      char_buffer_add (&charbuf, c);
> -	      /* It is "nan".  */
> +	      /* It is at least "nan".  Now we check for nan() and
> +	         nan(n-char-sequence).  */
> +	      if (width != 0 && inchar () != EOF)
> +		{
> +		  if (c == L_ ('('))

Currently style if not not add a whitespace for the macro, so just L_('(')


> +		    {
> +		      if (width > 0)
> +			--width;
> +		      char_buffer_add (&charbuf, c);
> +		      /* A '(' was observed, check for a closing ')', there
> +			 may or may not be a n-char-sequence in between.  We
> +			 have to check the longest prefix until there is a
> +			 conversion error or closing parenthesis.  */
> +		      do
> +			{
> +			  if (__builtin_expect (width == 0
> +						|| inchar () == EOF, 0))

Use __glibc_unlikely here.

> +			    {
> +			      /* Conversion error because we ran out of
> +				 characters.  */
> +			      conv_error ();
> +			      break;
> +			    }
> +			  if (!((c >= L_ ('0') && c <= L_ ('9'))
> +				|| (c >= L_ ('A') && c <= L_ ('Z'))
> +				|| (c >= L_ ('a') && c <= L_ ('z'))
> +				|| c == L_ ('_') || c == L_ (')')))

I am not sure if we can rely on this comparison for isalnum for all locales,
maybe a better strategy would be to do something like:

#ifdef COMPILE_WSCAN
# define ISXALNUM(Ch)     iswalnum (Ch)
#else
# define ISXALNUM(Ch)     __iswalnum_l (Ch, loc)
#endif
[...]
                          if (!(ISXALNUM (c) || c == L_('_') || c == L_(')')))
				

> +			    {
> +			      /* Invalid character was observed.  Only valid
> +				 characters are [a-zA-Z0-9_] and ')'.  */
> +			      conv_error ();
> +			      break;
> +			    }
> +			  if (width > 0)
> +			    --width;
> +			  char_buffer_add (&charbuf, c);
> +			}
> +		      while (c != L_ (')'));
> +		      /* The loop only exits successfully when ')' is the
> +			 last character.  */
> +		    }
> +		  else
> +		    /* It is only 'nan'.  */
> +		    ungetc (c, s);
> +		}
>  	      goto scan_float;
>  	    }
>  	  else if (TOLOWER (c) == L_('i'))

Fwd: [PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ #30647]

[Avinal Kumar]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

---------- Forwarded message ---------
From: Avinal Kumar <avinal.xlvii@gmail.com>
Date: Thu, Oct 24, 2024 at 1:33 PM
Subject: Re: [PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ
#30647]
To: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>




On Wed, Oct 23, 2024 at 11:07 PM Adhemerval Zanella Netto <
adhemerval.zanella@linaro.org> wrote:

>
> Maybe also check for float and long double as well?
>
Did you mean to check NaN formatted as float and long double?

>
>
> Currently style if not not add a whitespace for the macro, so just L_('(')
>
Ok

>
> Use __glibc_unlikely here.
>
Ok

>
> I am not sure if we can rely on this comparison for isalnum for all
> locales,
> maybe a better strategy would be to do something like:
>
> #ifdef COMPILE_WSCAN
> # define ISXALNUM(Ch)     iswalnum (Ch)
> #else
> # define ISXALNUM(Ch)     __iswalnum_l (Ch, loc)
> #endif
> [...]
>                           if (!(ISXALNUM (c) || c == L_('_') || c ==
> L_(')')))
>

Do we really want to check for all locales? I picked the check from
stdlib/strtod_nan_main.c (line 38), the comment mentions that the
n-char-sequence should be ASCII characters.

Thanks and Regards

Re: Fwd: [PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ #30647]

[Adhemerval Zanella Netto]

On 24/10/24 05:05, Avinal Kumar wrote:
> 
> 
> ---------- Forwarded message ---------
> From: *Avinal Kumar* <avinal.xlvii@gmail.com <mailto:avinal.xlvii@gmail.com>>
> Date: Thu, Oct 24, 2024 at 1:33 PM
> Subject: Re: [PATCH v2] stdio-common: Fix scanf parsing for NaN types [BZ #30647]
> To: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>>
> 
> 
> 
> 
> On Wed, Oct 23, 2024 at 11:07 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
> 
> 
>     Maybe also check for float and long double as well?
> 
> Did you mean to check NaN formatted as float and long double?

Yes, just to improve coverage.


> 
> 
> 
>     Currently style if not not add a whitespace for the macro, so just L_('(')
> 
> Ok
> 
> 
>     Use __glibc_unlikely here.
> 
> Ok
> 
> 
>     I am not sure if we can rely on this comparison for isalnum for all locales,
>     maybe a better strategy would be to do something like:
> 
>     #ifdef COMPILE_WSCAN
>     # define ISXALNUM(Ch)     iswalnum (Ch)
>     #else
>     # define ISXALNUM(Ch)     __iswalnum_l (Ch, loc)
>     #endif
>     [...]
>                               if (!(ISXALNUM (c) || c == L_('_') || c == L_(')')))
> 
>  
> Do we really want to check for all locales? I picked the check from  stdlib/strtod_nan_main.c (line 38), the comment mentions that the n-char-sequence should be ASCII characters.

Right, I think it fair to follow strtod here then.