[v2] time: Fix overflow itimer tests on 32-bit systems

  On the port of OpenRISC I am working on and it appears the rv32 port
we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
causing truncation.

The truncations are unavoidable on these systems so skip the
testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.

Also, futher in the tests and in other parts of code checking for time_t
overflow does not work on 32-bit systems when time_t is 64-bit.  As
suggested by Adhemerval, update the in_time_t_range function to assume
32-bits by using int32_t.

This also brings in the header for stdint.h so we can update other
usages of __int32_t to int32_t as suggested by Adhemerval.
---

Hello,

Sorry for the delay to get this out I have been busy on the hardware side of
openrisc the last month so I haven't been able to spend time on getting this
out.

The patch ends up doing a test fix and some lib code fixes, I can split it to
separate small patches.  But since as a whole it's small I feel leaving it
together is best.

-Stafford

 include/time.h    | 10 ++++++----
 time/tst-itimer.c |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)
  

On 06/08/2021 06:42, Stafford Horne wrote:
> On the port of OpenRISC I am working on and it appears the rv32 port
> we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
> size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
> causing truncation.
> 
> The truncations are unavoidable on these systems so skip the
> testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.
> 
> Also, futher in the tests and in other parts of code checking for time_t
> overflow does not work on 32-bit systems when time_t is 64-bit.  As
> suggested by Adhemerval, update the in_time_t_range function to assume
> 32-bits by using int32_t.
> 
> This also brings in the header for stdint.h so we can update other
> usages of __int32_t to int32_t as suggested by Adhemerval.
> ---
> 
> Hello,
> 
> Sorry for the delay to get this out I have been busy on the hardware side of
> openrisc the last month so I haven't been able to spend time on getting this
> out.
> 
> The patch ends up doing a test fix and some lib code fixes, I can split it to
> separate small patches.  But since as a whole it's small I feel leaving it
> together is best.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> -Stafford
> 
>  include/time.h    | 10 ++++++----
>  time/tst-itimer.c |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/time.h b/include/time.h
> index 4372bfbd96..ba3c5116cf 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -11,6 +11,7 @@
>  # include <sys/time.h>
>  # include <time-clockid.h>
>  # include <sys/time.h>
> +# include <stdint.h>
>  
>  extern __typeof (strftime_l) __strftime_l;
>  libc_hidden_proto (__strftime_l)
> @@ -334,11 +335,12 @@ libc_hidden_proto (__time64)
>     actual clock ID.  */
>  #define CLOCK_IDFIELD_SIZE	3
>  
> -/* Check whether T fits in time_t.  */
> +/* Check whether T fits in int32_t, assume all usages are for
> +   sizeof(time_t) == 32.  */
>  static inline bool
>  in_time_t_range (__time64_t t)
>  {
> -  time_t s = t;
> +  int32_t s = t;
>    return s == t;
>  }
>  
> @@ -445,8 +447,8 @@ timespec64_to_timeval64 (const struct __timespec64 ts64)
>     and suseconds_t.  */
>  struct __timeval32
>  {
> -  __int32_t tv_sec;         /* Seconds.  */
> -  __int32_t tv_usec;        /* Microseconds.  */
> +  int32_t tv_sec;         /* Seconds.  */
> +  int32_t tv_usec;        /* Microseconds.  */
>  };
>  
>  /* Conversion functions for converting to/from __timeval32  */
> diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> index 929c2b74c7..bd7d7afe83 100644
> --- a/time/tst-itimer.c
> +++ b/time/tst-itimer.c
> @@ -100,7 +100,7 @@ do_test (void)
>  
>        /* Linux does not provide 64 bit time_t support for getitimer and
>  	 setitimer on architectures with 32 bit time_t support.  */
> -      if (sizeof (__time_t) == 8)
> +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>  	{
>  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
>  	  TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 },
> @@ -131,7 +131,7 @@ do_test (void)
>        it.it_interval.tv_usec = 20;
>        it.it_value.tv_sec = 30;
>        it.it_value.tv_usec = 40;
> -      if (sizeof (__time_t) == 8)
> +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>  	{
>  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
>  
>
  

On Wed, Aug 11, 2021 at 05:30:40PM -0300, Adhemerval Zanella wrote:
> 
> 
> On 06/08/2021 06:42, Stafford Horne wrote:
> > On the port of OpenRISC I am working on and it appears the rv32 port
> > we have sets __TIMESIZE == 64 && __WORDSIZE == 32.  This causes the
> > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit
> > causing truncation.
> > 
> > The truncations are unavoidable on these systems so skip the
> > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64.
> > 
> > Also, futher in the tests and in other parts of code checking for time_t
> > overflow does not work on 32-bit systems when time_t is 64-bit.  As
> > suggested by Adhemerval, update the in_time_t_range function to assume
> > 32-bits by using int32_t.
> > 
> > This also brings in the header for stdint.h so we can update other
> > usages of __int32_t to int32_t as suggested by Adhemerval.
> > ---
> > 
> > Hello,
> > 
> > Sorry for the delay to get this out I have been busy on the hardware side of
> > openrisc the last month so I haven't been able to spend time on getting this
> > out.
> > 
> > The patch ends up doing a test fix and some lib code fixes, I can split it to
> > separate small patches.  But since as a whole it's small I feel leaving it
> > together is best.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

Thanks for the review.

> > 
> > -Stafford
> > 
> >  include/time.h    | 10 ++++++----
> >  time/tst-itimer.c |  4 ++--
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/time.h b/include/time.h
> > index 4372bfbd96..ba3c5116cf 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -11,6 +11,7 @@
> >  # include <sys/time.h>
> >  # include <time-clockid.h>
> >  # include <sys/time.h>
> > +# include <stdint.h>
> >  
> >  extern __typeof (strftime_l) __strftime_l;
> >  libc_hidden_proto (__strftime_l)
> > @@ -334,11 +335,12 @@ libc_hidden_proto (__time64)
> >     actual clock ID.  */
> >  #define CLOCK_IDFIELD_SIZE	3
> >  
> > -/* Check whether T fits in time_t.  */
> > +/* Check whether T fits in int32_t, assume all usages are for
> > +   sizeof(time_t) == 32.  */
> >  static inline bool
> >  in_time_t_range (__time64_t t)
> >  {
> > -  time_t s = t;
> > +  int32_t s = t;
> >    return s == t;
> >  }
> >  
> > @@ -445,8 +447,8 @@ timespec64_to_timeval64 (const struct __timespec64 ts64)
> >     and suseconds_t.  */
> >  struct __timeval32
> >  {
> > -  __int32_t tv_sec;         /* Seconds.  */
> > -  __int32_t tv_usec;        /* Microseconds.  */
> > +  int32_t tv_sec;         /* Seconds.  */
> > +  int32_t tv_usec;        /* Microseconds.  */
> >  };
> >  
> >  /* Conversion functions for converting to/from __timeval32  */
> > diff --git a/time/tst-itimer.c b/time/tst-itimer.c
> > index 929c2b74c7..bd7d7afe83 100644
> > --- a/time/tst-itimer.c
> > +++ b/time/tst-itimer.c
> > @@ -100,7 +100,7 @@ do_test (void)
> >  
> >        /* Linux does not provide 64 bit time_t support for getitimer and
> >  	 setitimer on architectures with 32 bit time_t support.  */
> > -      if (sizeof (__time_t) == 8)
> > +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
> >  	{
> >  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
> >  	  TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 },
> > @@ -131,7 +131,7 @@ do_test (void)
> >        it.it_interval.tv_usec = 20;
> >        it.it_value.tv_sec = 30;
> >        it.it_value.tv_usec = 40;
> > -      if (sizeof (__time_t) == 8)
> > +      if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
> >  	{
> >  	  TEST_COMPARE (setitimer (timers[i], &it, NULL), 0);
> >  
> >
  

I'm seeing a build failure in the glibc testsuite for i686-gnu:

tst-itimer.c: In function 'do_test':
tst-itimer.c:103:11: error: '__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64' undeclared (first use in this function)
  103 |       if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tst-itimer.c:103:11: note: each undeclared identifier is reported only once for each function it appears in

https://sourceware.org/pipermail/libc-testresults/2021q3/008412.html
  

On Mon, Aug 16, 2021 at 05:12:04PM +0000, Joseph Myers wrote:
> I'm seeing a build failure in the glibc testsuite for i686-gnu:
> 
> tst-itimer.c: In function 'do_test':
> tst-itimer.c:103:11: error: '__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64' undeclared (first use in this function)
>   103 |       if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64)
>       |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> tst-itimer.c:103:11: note: each undeclared identifier is reported only once for each function it appears in
> 
> https://sourceware.org/pipermail/libc-testresults/2021q3/008412.html

Right sorry about that, so __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 only exists in
linux.

So it looks like for non linux the new timer test changes break.

Should we provide __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 for not linux builds,
or remove __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 from the itimer test again?
The reason for using __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 is to pick up the
timeval size which is different on each architecture.

Maybe the easiest is adding something like:

#ifndef __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
#define __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 (sizeof (__time_t) == 8)
#endif

To the top of tst-itimer.c.  Sorry I don't have time right now to send a patch
or test this.

-Stafford
  

On Tue, 17 Aug 2021, Stafford Horne via Libc-alpha wrote:

> Should we provide __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 for not linux builds,
> or remove __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 from the itimer test again?
> The reason for using __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 is to pick up the
> timeval size which is different on each architecture.

I'd suggest having a macro that doesn't refer to either "kernel" or "old 
timeval" (and that is defined for both Linux and Hurd).  As far as I 
understand, the logical concept that's relevant for this test isn't either 
one of those, it's more like "setitimer supports times that cannot be 
represented in 32 bits".
  

On Tue, Aug 17, 2021, 7:33 AM Joseph Myers <joseph@codesourcery.com> wrote:

> On Tue, 17 Aug 2021, Stafford Horne via Libc-alpha wrote:
>
> > Should we provide __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 for not linux
> builds,
> > or remove __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 from the itimer test
> again?
> > The reason for using __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 is to pick
> up the
> > timeval size which is different on each architecture.
>
> I'd suggest having a macro that doesn't refer to either "kernel" or "old
> timeval" (and that is defined for both Linux and Hurd).  As far as I
> understand, the logical concept that's relevant for this test isn't either
> one of those, it's more like "setitimer supports times that cannot be
> represented in 32 bits".
>

Hello,

That makes sense, currently with the hurd build being broken how urgent is
this?  I worked on reproducing the build issue with build-many but didn't
get it working yet, probably about 80% there before I ran out of time.

I'll try to get it fixed in a few days as my top priority, but I only have
an hour or two a day to look at it. If we need to revert or add a temporary
patch please feel free.

-stafford

>
  

On 17/08/2021 20:33, Stafford Horne via Libc-alpha wrote:
> On Tue, Aug 17, 2021, 7:33 AM Joseph Myers <joseph@codesourcery.com> wrote:
> 
>> On Tue, 17 Aug 2021, Stafford Horne via Libc-alpha wrote:
>>
>>> Should we provide __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 for not linux
>> builds,
>>> or remove __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 from the itimer test
>> again?
>>> The reason for using __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 is to pick
>> up the
>>> timeval size which is different on each architecture.
>>
>> I'd suggest having a macro that doesn't refer to either "kernel" or "old
>> timeval" (and that is defined for both Linux and Hurd).  As far as I
>> understand, the logical concept that's relevant for this test isn't either
>> one of those, it's more like "setitimer supports times that cannot be
>> represented in 32 bits".
>>
> 
> Hello,
> 
> That makes sense, currently with the hurd build being broken how urgent is
> this?  I worked on reproducing the build issue with build-many but didn't
> get it working yet, probably about 80% there before I ran out of time.
> 
> I'll try to get it fixed in a few days as my top priority, but I only have
> an hour or two a day to look at it. If we need to revert or add a temporary
> patch please feel free.

The most straightforward would be to add a libsupport routine to abstract
what Joseph has suggested:

support/xtime.h:

  bool support_setitimer_support_64_bit (void);

And return 0 for hurd and __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 for Linux.