diff mbox series

Test errno setting on strtod overflow in tst-strtod-round

Message ID 988bfab3-349b-962f-4ba2-2bcb2051f6f4@redhat.com
State New
Headers show
Series Test errno setting on strtod overflow in tst-strtod-round | expand

Commit Message

Joseph Myers Aug. 13, 2024, 5:52 p.m. UTC
We have no tests that errno is set to ERANGE on overflow of
strtod-family functions (we do have some tests for underflow, in
tst-strtod-underflow).  Add such tests to tst-strtod-round.

Tested for x86_64.

Comments

Florian Weimer Aug. 14, 2024, 9:26 a.m. UTC | #1
* Joseph Myers:

> We have no tests that errno is set to ERANGE on overflow of
> strtod-family functions (we do have some tests for underflow, in
> tst-strtod-underflow).  Add such tests to tst-strtod-round.
>
> Tested for x86_64.
>
> diff --git a/stdlib/tst-strtod-round-skeleton.c b/stdlib/tst-strtod-round-skeleton.c
> index 6fba4b5228..184ec07b67 100644
> --- a/stdlib/tst-strtod-round-skeleton.c
> +++ b/stdlib/tst-strtod-round-skeleton.c
> @@ -21,6 +21,7 @@
>     declared in the headers.  */
>  #define _LIBC_TEST 1
>  #define __STDC_WANT_IEC_60559_TYPES_EXT__
> +#include <errno.h>
>  #include <fenv.h>
>  #include <float.h>
>  #include <math.h>
> @@ -205,7 +206,9 @@ struct test {
>  #define GEN_ONE_TEST(FSUF, FTYPE, FTOSTR, LSUF, CSUF)		\
>  {								\
>    feclearexcept (FE_ALL_EXCEPT);				\
> +  errno = 0;							\
>    FTYPE f = STRTO (FSUF) (s, NULL);				\
> +  int new_errno = errno;					\
>    if (f != expected->FSUF					\
>        || (copysign ## CSUF) (1.0 ## LSUF, f)			\
>  	 != (copysign ## CSUF) (1.0 ## LSUF, expected->FSUF))	\
> @@ -254,6 +257,13 @@ struct test {
>  		printf ("ignoring this exception error\n");	\
>  	    }							\
>  	}							\
> +      if (overflow->FSUF && new_errno != ERANGE)		\
> +	{							\
> +	  printf (FNPFXS "to" #FSUF				\
> +		  " (" STRM ") did not set errno to ERANGE\n",	\
> +		  s);						\
> +	  result = 1;						\
> +	}							\
>      }								\
>  }

Maybe print new_errno in the error message?  Rest looks okay.

Thanks,
Florian
Zack Weinberg Aug. 14, 2024, 5:25 p.m. UTC | #2
On Tue, Aug 13, 2024, at 1:52 PM, Joseph Myers wrote:
> We have no tests that errno is set to ERANGE on overflow of
> strtod-family functions (we do have some tests for underflow, in
> tst-strtod-underflow).  Add such tests to tst-strtod-round.

Maybe we should also have tests that errno is left unchanged by
these functions if no overflow or underflow occurred?  (I don't
know if these already exist.)

zw
Joseph Myers Aug. 14, 2024, 5:51 p.m. UTC | #3
On Wed, 14 Aug 2024, Zack Weinberg wrote:

> On Tue, Aug 13, 2024, at 1:52 PM, Joseph Myers wrote:
> > We have no tests that errno is set to ERANGE on overflow of
> > strtod-family functions (we do have some tests for underflow, in
> > tst-strtod-underflow).  Add such tests to tst-strtod-round.
> 
> Maybe we should also have tests that errno is left unchanged by
> these functions if no overflow or underflow occurred?  (I don't
> know if these already exist.)

The difficulty there is the details of "no underflow".  We do have 
tst-strtod-underflow that checks this for various edge cases of underflow 
(for double, I intend to extend it to cover other types as well).  We also 
have bug 32045 which notes we wrongly set errno for overflow in parsing a 
NaN payload.
Zack Weinberg Aug. 14, 2024, 6:10 p.m. UTC | #4
On Wed, Aug 14, 2024, at 1:51 PM, Joseph Myers wrote:
> On Wed, 14 Aug 2024, Zack Weinberg wrote:
>> On Tue, Aug 13, 2024, at 1:52 PM, Joseph Myers wrote:
>> > We have no tests that errno is set to ERANGE on overflow of strtod-
>> > family functions (we do have some tests for underflow, in tst-strtod-
>> > underflow).  Add such tests to tst-strtod-round.
>>
>> Maybe we should also have tests that errno is left unchanged by these
>> functions if no overflow or underflow occurred?  (I don't know if
>> these already exist.)
>
> The difficulty there is the details of "no underflow".  We do have tst-strtod-
> underflow that checks this for various edge cases of underflow (for
> double, I intend to extend it to cover other types as well).  We also
> have bug 32045 which notes we wrongly set errno for overflow in
> parsing a NaN payload.

I think the most valuable thing would be to check for unchanged errno
after parsing each of some uniformly distributed random sample of normal
numbers. Further, exploring the space of valid input strings is probably
more useful than exploring the space of normal-number return values, if
that makes sense?

zw
Maciej W. Rozycki Aug. 14, 2024, 9:48 p.m. UTC | #5
On Wed, 14 Aug 2024, Zack Weinberg wrote:

> >> Maybe we should also have tests that errno is left unchanged by these
> >> functions if no overflow or underflow occurred?  (I don't know if
> >> these already exist.)
> >
> > The difficulty there is the details of "no underflow".  We do have tst-strtod-
> > underflow that checks this for various edge cases of underflow (for
> > double, I intend to extend it to cover other types as well).  We also
> > have bug 32045 which notes we wrongly set errno for overflow in
> > parsing a NaN payload.
> 
> I think the most valuable thing would be to check for unchanged errno
> after parsing each of some uniformly distributed random sample of normal
> numbers. Further, exploring the space of valid input strings is probably
> more useful than exploring the space of normal-number return values, if
> that makes sense?

 Most importantly we might want to verify that a successful call to strtod 
does not reset errno to zero (that is where set to nonzero beforehand), as 
required by ISO C and POSIX.

  Maciej
Zack Weinberg Aug. 15, 2024, 12:38 a.m. UTC | #6
On Wed, Aug 14, 2024, at 9:48 PM, Maciej W. Rozycki wrote:
> On Wed, 14 Aug 2024, Zack Weinberg wrote:
>> >> Maybe we should also have tests that errno is left unchanged by
>> >> these functions if no overflow or underflow occurred?  (I don't
>> >> know if these already exist.)
>> >
>> > The difficulty there is the details of "no underflow".  We do have
>> > tst-strtod- underflow that checks this for various edge cases of
>> > underflow (for double, I intend to extend it to cover other types
>> > as well).  We also have bug 32045 which notes we wrongly set errno
>> > for overflow in parsing a NaN payload.
>>
>> I think the most valuable thing would be to check for unchanged errno
>> after parsing each of some uniformly distributed random sample of
>> normal numbers. Further, exploring the space of valid input strings
>> is probably more useful than exploring the space of normal-number
>> return values, if that makes sense?
>
>  Most importantly we might want to verify that a successful call to
>  strtod does not reset errno to zero (that is where set to nonzero
>  beforehand), as required by ISO C and POSIX.

That's a requirement for all C library functions - and one that we
probably should be testing, in general - but for the strto* family the
requirement is stronger.  As you probably already know, most C library
functions are allowed to set errno to an arbitrary nonzero value *even
if* they succeed.  But strto* must not alter errno at all when they
succeed, because some error conditions for strto* are *only* reported by
setting errno to a nonzero value.

Thus the most important cases to test are the success cases that collide
with the value strto* returns on error: that is, *successful* parsing of
LONG_MAX for strtol, ULONG_MAX for strtoul, HUGE_VAL for strtod, etc.

zw
Maciej W. Rozycki Aug. 15, 2024, 1:40 p.m. UTC | #7
On Thu, 15 Aug 2024, Zack Weinberg wrote:

> >> I think the most valuable thing would be to check for unchanged errno
> >> after parsing each of some uniformly distributed random sample of
> >> normal numbers. Further, exploring the space of valid input strings
> >> is probably more useful than exploring the space of normal-number
> >> return values, if that makes sense?
> >
> >  Most importantly we might want to verify that a successful call to
> >  strtod does not reset errno to zero (that is where set to nonzero
> >  beforehand), as required by ISO C and POSIX.
> 
> That's a requirement for all C library functions - and one that we
> probably should be testing, in general - but for the strto* family the
> requirement is stronger.  As you probably already know, most C library
> functions are allowed to set errno to an arbitrary nonzero value *even
> if* they succeed.  But strto* must not alter errno at all when they
> succeed, because some error conditions for strto* are *only* reported by
> setting errno to a nonzero value.

 That is true, but given that strto* functions return no error status 
(other than setting errno) it might be tempting for an implementer to 
reset errno to zero on success.

> Thus the most important cases to test are the success cases that collide
> with the value strto* returns on error: that is, *successful* parsing of
> LONG_MAX for strtol, ULONG_MAX for strtoul, HUGE_VAL for strtod, etc.

 Agreed.  This includes zero as well, which is also returned where no 
conversion could be performed (and for which POSIX permits errno to be set 
to EINVAL).

  Maciej
diff mbox series

Patch

diff --git a/stdlib/tst-strtod-round-skeleton.c b/stdlib/tst-strtod-round-skeleton.c
index 6fba4b5228..184ec07b67 100644
--- a/stdlib/tst-strtod-round-skeleton.c
+++ b/stdlib/tst-strtod-round-skeleton.c
@@ -21,6 +21,7 @@ 
    declared in the headers.  */
 #define _LIBC_TEST 1
 #define __STDC_WANT_IEC_60559_TYPES_EXT__
+#include <errno.h>
 #include <fenv.h>
 #include <float.h>
 #include <math.h>
@@ -205,7 +206,9 @@  struct test {
 #define GEN_ONE_TEST(FSUF, FTYPE, FTOSTR, LSUF, CSUF)		\
 {								\
   feclearexcept (FE_ALL_EXCEPT);				\
+  errno = 0;							\
   FTYPE f = STRTO (FSUF) (s, NULL);				\
+  int new_errno = errno;					\
   if (f != expected->FSUF					\
       || (copysign ## CSUF) (1.0 ## LSUF, f)			\
 	 != (copysign ## CSUF) (1.0 ## LSUF, expected->FSUF))	\
@@ -254,6 +257,13 @@  struct test {
 		printf ("ignoring this exception error\n");	\
 	    }							\
 	}							\
+      if (overflow->FSUF && new_errno != ERANGE)		\
+	{							\
+	  printf (FNPFXS "to" #FSUF				\
+		  " (" STRM ") did not set errno to ERANGE\n",	\
+		  s);						\
+	  result = 1;						\
+	}							\
     }								\
 }