[v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

  The nanosleep syscall is not supported on newer 32-bit platforms (such
as RV32). To fix this issue let's use clock_nanosleep_time64 if it is
avaliable.

Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as the
Linux specification says:
  "POSIX.1 specifies that nanosleep() should measure time against the
   CLOCK_REALTIME clock. However, Linux measures the time using the
   CLOCK_MONOTONIC clock. This probably does not matter, since the POSIX.1
   specification for clock_settime(2) says that discontinuous changes in
   CLOCK_REALTIME should not affect nanosleep()"
---
This was patch was runtime tested with RV32 and RV64
It was build tested using the ./scripts/build-many-glibcs.py script.

I am currently running:
$ make ; make install ; make check
tests on native ARM (32-bit) with the following three confiugrations:
 - 4.19 Kernel and 4.19 Headers
 - 5.2 Kernel and 4.19 Headers
 - 5.2 Kernel and 5.2 Headers
I should have the results tomorrow.

v2:
 - Explicitly include `#include <kernel-features.h>`

 include/time.h                               | 20 ++++++
 nptl/thrd_sleep.c                            | 70 +++++++++++++++++--
 sysdeps/unix/sysv/linux/clock_nanosleep.c    | 73 ++++++++++++++++++--
 sysdeps/unix/sysv/linux/nanosleep.c          | 65 ++++++++++++++++-
 sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 65 ++++++++++++++++-
 5 files changed, 280 insertions(+), 13 deletions(-)
  

On Thu, 24 Oct 2019, Alistair Francis wrote:

> +# ifdef __NR_clock_nanosleep_time64
> +  long int ret_64;
> +
> +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                    0, time_point, remaining);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    ret = ret_64;
> +# endif /* __NR_clock_nanosleep_time64 */
> +  if (ret < 0)
> +    {
> +      struct timespec tp32, tr32;
> +
> +      if (! in_time_t_range (time_point->tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      tp32 = valid_timespec64_to_timespec (*time_point);
> +      ret =  INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32);

This looks like it would call the nanosleep syscall if 
clock_nanosleep_time64 failed for some non-ENOSYS reason.  That doesn't 
seem right.

> +      if ((ret == 0 || errno != ENOSYS) && remaining)
> +        *remaining = valid_timespec_to_timespec64 (tr32);

And if nanosleep fails for a non-EINTR reason, it won't have filled in 
tr32, so this could be copying uninitialized data.

> +#if __TIMESIZE != 64
> +int
> +thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> +{
> +  int ret;
> +  struct __timespec64 tp64, tr64;
> +
> +  tp64 = valid_timespec_to_timespec64 (*time_point);
> +  ret = __thrd_sleep_time64 (&tp64, &tr64);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      if (! in_time_t_range (tr64.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;

This could not only be copying uninitialized data for such a failure (e.g. 
a failure for an invalid nanoseconds value), it could actually produce a 
spurious EOVERFLOW error if that uninitialized data is out of range.

As far as I know the remaining amount can't exceed the amount passed in, 
so when it's copied it shouldn't actually be necessary to check for 
overflow.

I think similar issues apply for other functions in the patch.

Do we have test coverage in the testsuite for invalid nanoseconds 
arguments to all these functions, making sure they produce proper EINVAL 
errors?  That might not cover many of these bugs (it wouldn't detect 
simply reading uninitialized memory, or calling more syscalls than 
necessary, and detecting the spurious EOVERFLOW would depend on what 
values were in that memory).  But it would seem a good idea to make sure 
we do have such test coverage for functions using time arguments.
  

On 24/10/2019 20:41, Alistair Francis wrote:
> The nanosleep syscall is not supported on newer 32-bit platforms (such
> as RV32). To fix this issue let's use clock_nanosleep_time64 if it is
> avaliable.
> 
> Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as the
> Linux specification says:
>   "POSIX.1 specifies that nanosleep() should measure time against the
>    CLOCK_REALTIME clock. However, Linux measures the time using the
>    CLOCK_MONOTONIC clock. This probably does not matter, since the POSIX.1
>    specification for clock_settime(2) says that discontinuous changes in
>    CLOCK_REALTIME should not affect nanosleep()"
> ---
> This was patch was runtime tested with RV32 and RV64
> It was build tested using the ./scripts/build-many-glibcs.py script.
> 
> I am currently running:
> $ make ; make install ; make check
> tests on native ARM (32-bit) with the following three confiugrations:
>  - 4.19 Kernel and 4.19 Headers
>  - 5.2 Kernel and 4.19 Headers
>  - 5.2 Kernel and 5.2 Headers
> I should have the results tomorrow.
> 
> v2:
>  - Explicitly include `#include <kernel-features.h>`
> 
>  include/time.h                               | 20 ++++++
>  nptl/thrd_sleep.c                            | 70 +++++++++++++++++--
>  sysdeps/unix/sysv/linux/clock_nanosleep.c    | 73 ++++++++++++++++++--
>  sysdeps/unix/sysv/linux/nanosleep.c          | 65 ++++++++++++++++-
>  sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 65 ++++++++++++++++-
>  5 files changed, 280 insertions(+), 13 deletions(-)
> 
> diff --git a/include/time.h b/include/time.h
> index d93b16a7810..0e703f87cef 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -174,6 +174,26 @@ libc_hidden_proto (__difftime64)
>  
>  extern double __difftime (time_t time1, time_t time0);
>  
> +#if __TIMESIZE == 64
> +# define __thrd_sleep_time64 thrd_sleep
> +# define __clock_nanosleep_time64 __clock_nanosleep
> +# define __nanosleep_time64 __nanosleep
> +# define __nanosleep_nocancel_time64 __nanosleep_nocancel
> +#else
> +extern int __thrd_sleep_time64 (const struct __timespec64* time_point,
> +                                struct __timespec64* remaining);
> +libc_hidden_proto (__thrd_sleep_time64)
> +extern int __clock_nanosleep_time64 (clockid_t clock_id,
> +                                     int flags, const struct __timespec64 *req,
> +                                     struct __timespec64 *rem);
> +libc_hidden_proto (__clock_nanosleep_time64)
> +extern  int __nanosleep_time64 (const struct __timespec64 *requested_time,
> +                                struct __timespec64 *remaining);
> +libc_hidden_proto (__nanosleep_time64)
> +extern int __nanosleep_nocancel_time64 (const struct __timespec64 *requested_time,
> +                                        struct __timespec64 *remaining);
> +libc_hidden_proto (__nanosleep_nocancel_time64)
> +#endif
>  
>  /* Use in the clock_* functions.  Size of the field representing the
>     actual clock ID.  */
> diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> index 2e185dd748e..2a6bb1f9e3f 100644
> --- a/nptl/thrd_sleep.c
> +++ b/nptl/thrd_sleep.c
> @@ -17,23 +17,85 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <time.h>
> +#include <kernel-features.h>
>  #include <sysdep-cancel.h>
>  
>  #include "thrd_priv.h"
>  
>  int
> -thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> +__thrd_sleep_time64 (const struct __timespec64* time_point, struct __timespec64* remaining)
>  {
>    INTERNAL_SYSCALL_DECL (err);
> -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> +  int ret = -1;
> +
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +# ifndef __NR_clock_nanosleep_time64
> +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> +# endif
> +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                 0, time_point, remaining);
> +#else
> +# ifdef __NR_clock_nanosleep_time64
> +  long int ret_64;
> +
> +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                    0, time_point, remaining);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    ret = ret_64;
> +# endif /* __NR_clock_nanosleep_time64 */
> +  if (ret < 0)
> +    {
> +      struct timespec tp32, tr32;
> +
> +      if (! in_time_t_range (time_point->tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      tp32 = valid_timespec64_to_timespec (*time_point);
> +      ret =  INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32);
> +
> +      if ((ret == 0 || errno != ENOSYS) && remaining)
> +        *remaining = valid_timespec_to_timespec64 (tr32);
> +    }
> +#endif /* __ASSUME_TIME64_SYSCALLS */
> +
>    if (INTERNAL_SYSCALL_ERROR_P (ret, err))
>      {
>        /* C11 states thrd_sleep function returns -1 if it has been interrupted
> -	 by a signal, or a negative value if it fails.  */
> +         by a signal, or a negative value if it fails.  */
>        ret = INTERNAL_SYSCALL_ERRNO (ret, err);
>        if (ret == EINTR)
> -	return -1;
> +        return -1;
>        return -2;
>      }
>    return 0;
>  }

Wouldn't be simple to just call __clock_nanosleep_time64 instead of duplicate
the syscall handling and the required error code for c11 thread?  Something
like:

int
__thrd_sleep_time64 (const struct __timespec64* time_point,
		     struct __timespec64* remaining)
{
  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point, remaining);
  /* C11 states thrd_sleep function returns -1 if it has been interrupted
     by a signal, or a negative value if it fails.  */
  switch (ret)
  {
     case 0:      return 0;
     case EINTR:  return -1;
     default:     return -2;
  }
}

> +
> +#if __TIMESIZE != 64
> +int
> +thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> +{
> +  int ret;
> +  struct __timespec64 tp64, tr64;
> +
> +  tp64 = valid_timespec_to_timespec64 (*time_point);
> +  ret = __thrd_sleep_time64 (&tp64, &tr64);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      if (! in_time_t_range (tr64.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      if (remaining)
> +        *remaining = valid_timespec64_to_timespec (tr64);
> +    }
> +
> +  return ret;
> +}
> +#endif
> diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c
> index 1f240b8720a..d257ea57fb0 100644
> --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c
> +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c
> @@ -16,6 +16,7 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <time.h>
> +#include <kernel-features.h>
>  #include <errno.h>
>  
>  #include <sysdep-cancel.h>
> @@ -26,9 +27,11 @@
>  /* We can simply use the syscall.  The CPU clocks are not supported
>     with this function.  */
>  int
> -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
> -		   struct timespec *rem)
> +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req,
> +                          struct __timespec64 *rem)
>  {
> +  int r = -1;
> +
>    if (clock_id == CLOCK_THREAD_CPUTIME_ID)
>      return EINVAL;
>    if (clock_id == CLOCK_PROCESS_CPUTIME_ID)
> @@ -37,12 +40,72 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
>    /* If the call is interrupted by a signal handler or encounters an error,
>       it returns a positive value similar to errno.  */
>    INTERNAL_SYSCALL_DECL (err);
> -  int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,
> -				   req, rem);
> +
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +# ifndef __NR_clock_nanosleep_time64
> +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> +# endif
> +  r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,
> +                               flags, req, rem);
> +#else
> +# ifdef __NR_clock_nanosleep_time64
> +  long int ret_64;
> +
> +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,
> +                                    flags, req, rem);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    r = ret_64;
> +# endif /* __NR_clock_nanosleep_time64 */
> +  if (r < 0)
> +    {
> +      struct timespec ts32, tr32;
> +
> +      if (! in_time_t_range (req->tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      ts32 = valid_timespec64_to_timespec (*req);
> +      r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32, &tr32);
> +
> +      if ((r == 0 || errno != ENOSYS) && rem)
> +        *rem = valid_timespec_to_timespec64 (tr32);
> +    }
> +#endif /* __ASSUME_TIME64_SYSCALLS */
> +
>    return (INTERNAL_SYSCALL_ERROR_P (r, err)
> -	  ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
> +          ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
>  }
>  
> +#if __TIMESIZE != 64
> +int
> +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
> +                   struct timespec *rem)
> +{
> +  int r;
> +  struct __timespec64 treq64, trem64;
> +
> +  treq64 = valid_timespec_to_timespec64 (*req);
> +  r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64);
> +
> +  if (r == 0 || errno != ENOSYS)
> +    {
> +      if (! in_time_t_range (trem64.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      if (rem)
> +        *rem = valid_timespec64_to_timespec (trem64);
> +    }
> +
> +  return r;
> +}
> +#endif
> +
>  versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17);
>  /* clock_nanosleep moved to libc in version 2.17;
>     old binaries may expect the symbol version it had in librt.  */
> diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c
> index 6787909248f..10a6d57a1de 100644
> --- a/sysdeps/unix/sysv/linux/nanosleep.c
> +++ b/sysdeps/unix/sysv/linux/nanosleep.c
> @@ -17,15 +17,76 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <time.h>
> +#include <kernel-features.h>
>  #include <sysdep-cancel.h>
>  #include <not-cancel.h>
>  
>  /* Pause execution for a number of nanoseconds.  */
>  int
> +__nanosleep_time64 (const struct __timespec64 *requested_time,
> +                    struct __timespec64 *remaining)
> +{
> +#if defined(__ASSUME_TIME64_SYSCALLS)
> +# ifndef __NR_clock_nanosleep_time64
> +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> +# endif
> +  return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> +                         requested_time, remaining);
> +#else
> +# ifdef __NR_clock_nanosleep_time64
> +  long int ret_64;
> +
> +  ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> +                           requested_time, remaining);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    return ret_64;
> +# endif /* __NR_clock_nanosleep_time64 */
> +  int ret;
> +  struct timespec ts32, tr32;
> +
> +  if (! in_time_t_range (requested_time->tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +
> +  ts32 = valid_timespec64_to_timespec (*requested_time);
> +  ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32);
> +
> +  if ((ret == 0 || errno != ENOSYS) && remaining)
> +    *remaining = valid_timespec_to_timespec64 (tr32);
> +
> +  return ret;
> +#endif /* __ASSUME_TIME64_SYSCALLS */
> +}
> +
> +#if __TIMESIZE != 64
> +int
>  __nanosleep (const struct timespec *requested_time,
> -	     struct timespec *remaining)
> +             struct timespec *remaining)
>  {
> -  return SYSCALL_CANCEL (nanosleep, requested_time, remaining);
> +  int r;
> +  struct __timespec64 treq64, trem64;
> +
> +  treq64 = valid_timespec_to_timespec64 (*requested_time);
> +  r = __nanosleep_time64 (&treq64, &trem64);
> +
> +  if (r == 0 || errno != ENOSYS)
> +    {
> +      if (! in_time_t_range (trem64.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      if (remaining)
> +        *remaining = valid_timespec64_to_timespec (trem64);
> +    }
> +
> +  return r;
>  }
> +#endif
> +
>  hidden_def (__nanosleep)
>  weak_alias (__nanosleep, nanosleep)
> diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> index d6442bf4f7f..1a6c6c0a48a 100644
> --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> @@ -17,13 +17,74 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <time.h>
> +#include <kernel-features.h>
>  #include <sysdep-cancel.h>
>  #include <not-cancel.h>
>  
> +int
> +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time,
> +                             struct __timespec64 *remaining)
> +{
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +# ifndef __NR_clock_nanosleep_time64
> +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> +# endif
> +  return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> +                              requested_time, remaining);
> +#else
> +# ifdef __NR_clock_nanosleep_time64
> +  long int ret_64;
> +
> +  ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> +                                requested_time, remaining);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    return ret_64;
> +# endif /* __NR_clock_nanosleep_time64 */
> +  int ret;
> +  struct timespec ts32, tr32;
> +
> +  if (! in_time_t_range (requested_time->tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +
> +  ts32 = valid_timespec64_to_timespec (*requested_time);
> +  ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    *remaining = valid_timespec_to_timespec64 (tr32);
> +
> +  return ret;
> +#endif /* __ASSUME_TIME64_SYSCALLS */
> +}
> +
> +#if __TIMESIZE != 64
>  int
>  __nanosleep_nocancel (const struct timespec *requested_time,
> -		      struct timespec *remaining)
> +                      struct timespec *remaining)
>  {
> -  return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining);
> +  int ret;
> +  struct __timespec64 treq64, trem64;
> +
> +  treq64 = valid_timespec_to_timespec64 (*requested_time);
> +  ret = __nanosleep_nocancel_time64 (&treq64, &trem64);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      if (! in_time_t_range (trem64.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      if (remaining)
> +        *remaining = valid_timespec64_to_timespec (trem64);
> +    }
> +
> +  return ret;
>  }
> +#endif
> +
>  hidden_def (__nanosleep_nocancel)
>
  

On Fri, Oct 25, 2019 at 11:44 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 24 Oct 2019, Alistair Francis wrote:
>
> > +# ifdef __NR_clock_nanosleep_time64
> > +  long int ret_64;
> > +
> > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                    0, time_point, remaining);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    ret = ret_64;
> > +# endif /* __NR_clock_nanosleep_time64 */
> > +  if (ret < 0)
> > +    {
> > +      struct timespec tp32, tr32;
> > +
> > +      if (! in_time_t_range (time_point->tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      tp32 = valid_timespec64_to_timespec (*time_point);
> > +      ret =  INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32);
>
> This looks like it would call the nanosleep syscall if
> clock_nanosleep_time64 failed for some non-ENOSYS reason.  That doesn't
> seem right.

Yes, you are right. I have fixed this in clock_nanosleep and the
conversion in thrd_sleep() pointed out by Adhemerval fixes it there.


>
> > +      if ((ret == 0 || errno != ENOSYS) && remaining)
> > +        *remaining = valid_timespec_to_timespec64 (tr32);
>
> And if nanosleep fails for a non-EINTR reason, it won't have filled in
> tr32, so this could be copying uninitialized data.

Yes, fixed.

>
> > +#if __TIMESIZE != 64
> > +int
> > +thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> > +{
> > +  int ret;
> > +  struct __timespec64 tp64, tr64;
> > +
> > +  tp64 = valid_timespec_to_timespec64 (*time_point);
> > +  ret = __thrd_sleep_time64 (&tp64, &tr64);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    {
> > +      if (! in_time_t_range (tr64.tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
>
> This could not only be copying uninitialized data for such a failure (e.g.
> a failure for an invalid nanoseconds value), it could actually produce a
> spurious EOVERFLOW error if that uninitialized data is out of range.
>
> As far as I know the remaining amount can't exceed the amount passed in,
> so when it's copied it shouldn't actually be necessary to check for
> overflow.

I have removed the check when converting the remaining time from
64-bit to 32-bit for all files.

>
> I think similar issues apply for other functions in the patch.
>
> Do we have test coverage in the testsuite for invalid nanoseconds
> arguments to all these functions, making sure they produce proper EINVAL
> errors?  That might not cover many of these bugs (it wouldn't detect
> simply reading uninitialized memory, or calling more syscalls than
> necessary, and detecting the spurious EOVERFLOW would depend on what
> values were in that memory).  But it would seem a good idea to make sure
> we do have such test coverage for functions using time arguments.

I did see test failures (they were running as I sent this patch) so I
think this is covered, although I haven't investigated the failures.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
  

On Mon, Oct 28, 2019 at 8:51 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 24/10/2019 20:41, Alistair Francis wrote:
> > The nanosleep syscall is not supported on newer 32-bit platforms (such
> > as RV32). To fix this issue let's use clock_nanosleep_time64 if it is
> > avaliable.
> >
> > Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as the
> > Linux specification says:
> >   "POSIX.1 specifies that nanosleep() should measure time against the
> >    CLOCK_REALTIME clock. However, Linux measures the time using the
> >    CLOCK_MONOTONIC clock. This probably does not matter, since the POSIX.1
> >    specification for clock_settime(2) says that discontinuous changes in
> >    CLOCK_REALTIME should not affect nanosleep()"
> > ---
> > This was patch was runtime tested with RV32 and RV64
> > It was build tested using the ./scripts/build-many-glibcs.py script.
> >
> > I am currently running:
> > $ make ; make install ; make check
> > tests on native ARM (32-bit) with the following three confiugrations:
> >  - 4.19 Kernel and 4.19 Headers
> >  - 5.2 Kernel and 4.19 Headers
> >  - 5.2 Kernel and 5.2 Headers
> > I should have the results tomorrow.
> >
> > v2:
> >  - Explicitly include `#include <kernel-features.h>`
> >
> >  include/time.h                               | 20 ++++++
> >  nptl/thrd_sleep.c                            | 70 +++++++++++++++++--
> >  sysdeps/unix/sysv/linux/clock_nanosleep.c    | 73 ++++++++++++++++++--
> >  sysdeps/unix/sysv/linux/nanosleep.c          | 65 ++++++++++++++++-
> >  sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 65 ++++++++++++++++-
> >  5 files changed, 280 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/time.h b/include/time.h
> > index d93b16a7810..0e703f87cef 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -174,6 +174,26 @@ libc_hidden_proto (__difftime64)
> >
> >  extern double __difftime (time_t time1, time_t time0);
> >
> > +#if __TIMESIZE == 64
> > +# define __thrd_sleep_time64 thrd_sleep
> > +# define __clock_nanosleep_time64 __clock_nanosleep
> > +# define __nanosleep_time64 __nanosleep
> > +# define __nanosleep_nocancel_time64 __nanosleep_nocancel
> > +#else
> > +extern int __thrd_sleep_time64 (const struct __timespec64* time_point,
> > +                                struct __timespec64* remaining);
> > +libc_hidden_proto (__thrd_sleep_time64)
> > +extern int __clock_nanosleep_time64 (clockid_t clock_id,
> > +                                     int flags, const struct __timespec64 *req,
> > +                                     struct __timespec64 *rem);
> > +libc_hidden_proto (__clock_nanosleep_time64)
> > +extern  int __nanosleep_time64 (const struct __timespec64 *requested_time,
> > +                                struct __timespec64 *remaining);
> > +libc_hidden_proto (__nanosleep_time64)
> > +extern int __nanosleep_nocancel_time64 (const struct __timespec64 *requested_time,
> > +                                        struct __timespec64 *remaining);
> > +libc_hidden_proto (__nanosleep_nocancel_time64)
> > +#endif
> >
> >  /* Use in the clock_* functions.  Size of the field representing the
> >     actual clock ID.  */
> > diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> > index 2e185dd748e..2a6bb1f9e3f 100644
> > --- a/nptl/thrd_sleep.c
> > +++ b/nptl/thrd_sleep.c
> > @@ -17,23 +17,85 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > +#include <kernel-features.h>
> >  #include <sysdep-cancel.h>
> >
> >  #include "thrd_priv.h"
> >
> >  int
> > -thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> > +__thrd_sleep_time64 (const struct __timespec64* time_point, struct __timespec64* remaining)
> >  {
> >    INTERNAL_SYSCALL_DECL (err);
> > -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> > +  int ret = -1;
> > +
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +# ifndef __NR_clock_nanosleep_time64
> > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > +# endif
> > +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                 0, time_point, remaining);
> > +#else
> > +# ifdef __NR_clock_nanosleep_time64
> > +  long int ret_64;
> > +
> > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> > +                                    0, time_point, remaining);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    ret = ret_64;
> > +# endif /* __NR_clock_nanosleep_time64 */
> > +  if (ret < 0)
> > +    {
> > +      struct timespec tp32, tr32;
> > +
> > +      if (! in_time_t_range (time_point->tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      tp32 = valid_timespec64_to_timespec (*time_point);
> > +      ret =  INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32);
> > +
> > +      if ((ret == 0 || errno != ENOSYS) && remaining)
> > +        *remaining = valid_timespec_to_timespec64 (tr32);
> > +    }
> > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > +
> >    if (INTERNAL_SYSCALL_ERROR_P (ret, err))
> >      {
> >        /* C11 states thrd_sleep function returns -1 if it has been interrupted
> > -      by a signal, or a negative value if it fails.  */
> > +         by a signal, or a negative value if it fails.  */
> >        ret = INTERNAL_SYSCALL_ERRNO (ret, err);
> >        if (ret == EINTR)
> > -     return -1;
> > +        return -1;
> >        return -2;
> >      }
> >    return 0;
> >  }
>
> Wouldn't be simple to just call __clock_nanosleep_time64 instead of duplicate
> the syscall handling and the required error code for c11 thread?  Something
> like:
>
> int
> __thrd_sleep_time64 (const struct __timespec64* time_point,
>                      struct __timespec64* remaining)
> {
>   int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point, remaining);
>   /* C11 states thrd_sleep function returns -1 if it has been interrupted
>      by a signal, or a negative value if it fails.  */
>   switch (ret)
>   {
>      case 0:      return 0;
>      case EINTR:  return -1;
>      default:     return -2;
>   }
> }

This looks good to me, I'll try that.

Alistair

>
> > +
> > +#if __TIMESIZE != 64
> > +int
> > +thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
> > +{
> > +  int ret;
> > +  struct __timespec64 tp64, tr64;
> > +
> > +  tp64 = valid_timespec_to_timespec64 (*time_point);
> > +  ret = __thrd_sleep_time64 (&tp64, &tr64);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    {
> > +      if (! in_time_t_range (tr64.tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      if (remaining)
> > +        *remaining = valid_timespec64_to_timespec (tr64);
> > +    }
> > +
> > +  return ret;
> > +}
> > +#endif
> > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c
> > index 1f240b8720a..d257ea57fb0 100644
> > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c
> > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c
> > @@ -16,6 +16,7 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > +#include <kernel-features.h>
> >  #include <errno.h>
> >
> >  #include <sysdep-cancel.h>
> > @@ -26,9 +27,11 @@
> >  /* We can simply use the syscall.  The CPU clocks are not supported
> >     with this function.  */
> >  int
> > -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
> > -                struct timespec *rem)
> > +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req,
> > +                          struct __timespec64 *rem)
> >  {
> > +  int r = -1;
> > +
> >    if (clock_id == CLOCK_THREAD_CPUTIME_ID)
> >      return EINVAL;
> >    if (clock_id == CLOCK_PROCESS_CPUTIME_ID)
> > @@ -37,12 +40,72 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
> >    /* If the call is interrupted by a signal handler or encounters an error,
> >       it returns a positive value similar to errno.  */
> >    INTERNAL_SYSCALL_DECL (err);
> > -  int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags,
> > -                                req, rem);
> > +
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +# ifndef __NR_clock_nanosleep_time64
> > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > +# endif
> > +  r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,
> > +                               flags, req, rem);
> > +#else
> > +# ifdef __NR_clock_nanosleep_time64
> > +  long int ret_64;
> > +
> > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id,
> > +                                    flags, req, rem);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    r = ret_64;
> > +# endif /* __NR_clock_nanosleep_time64 */
> > +  if (r < 0)
> > +    {
> > +      struct timespec ts32, tr32;
> > +
> > +      if (! in_time_t_range (req->tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      ts32 = valid_timespec64_to_timespec (*req);
> > +      r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32, &tr32);
> > +
> > +      if ((r == 0 || errno != ENOSYS) && rem)
> > +        *rem = valid_timespec_to_timespec64 (tr32);
> > +    }
> > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > +
> >    return (INTERNAL_SYSCALL_ERROR_P (r, err)
> > -       ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
> > +          ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
> >  }
> >
> > +#if __TIMESIZE != 64
> > +int
> > +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
> > +                   struct timespec *rem)
> > +{
> > +  int r;
> > +  struct __timespec64 treq64, trem64;
> > +
> > +  treq64 = valid_timespec_to_timespec64 (*req);
> > +  r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64);
> > +
> > +  if (r == 0 || errno != ENOSYS)
> > +    {
> > +      if (! in_time_t_range (trem64.tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      if (rem)
> > +        *rem = valid_timespec64_to_timespec (trem64);
> > +    }
> > +
> > +  return r;
> > +}
> > +#endif
> > +
> >  versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17);
> >  /* clock_nanosleep moved to libc in version 2.17;
> >     old binaries may expect the symbol version it had in librt.  */
> > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c
> > index 6787909248f..10a6d57a1de 100644
> > --- a/sysdeps/unix/sysv/linux/nanosleep.c
> > +++ b/sysdeps/unix/sysv/linux/nanosleep.c
> > @@ -17,15 +17,76 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > +#include <kernel-features.h>
> >  #include <sysdep-cancel.h>
> >  #include <not-cancel.h>
> >
> >  /* Pause execution for a number of nanoseconds.  */
> >  int
> > +__nanosleep_time64 (const struct __timespec64 *requested_time,
> > +                    struct __timespec64 *remaining)
> > +{
> > +#if defined(__ASSUME_TIME64_SYSCALLS)
> > +# ifndef __NR_clock_nanosleep_time64
> > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > +# endif
> > +  return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> > +                         requested_time, remaining);
> > +#else
> > +# ifdef __NR_clock_nanosleep_time64
> > +  long int ret_64;
> > +
> > +  ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> > +                           requested_time, remaining);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    return ret_64;
> > +# endif /* __NR_clock_nanosleep_time64 */
> > +  int ret;
> > +  struct timespec ts32, tr32;
> > +
> > +  if (! in_time_t_range (requested_time->tv_sec))
> > +    {
> > +      __set_errno (EOVERFLOW);
> > +      return -1;
> > +    }
> > +
> > +  ts32 = valid_timespec64_to_timespec (*requested_time);
> > +  ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32);
> > +
> > +  if ((ret == 0 || errno != ENOSYS) && remaining)
> > +    *remaining = valid_timespec_to_timespec64 (tr32);
> > +
> > +  return ret;
> > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > +}
> > +
> > +#if __TIMESIZE != 64
> > +int
> >  __nanosleep (const struct timespec *requested_time,
> > -          struct timespec *remaining)
> > +             struct timespec *remaining)
> >  {
> > -  return SYSCALL_CANCEL (nanosleep, requested_time, remaining);
> > +  int r;
> > +  struct __timespec64 treq64, trem64;
> > +
> > +  treq64 = valid_timespec_to_timespec64 (*requested_time);
> > +  r = __nanosleep_time64 (&treq64, &trem64);
> > +
> > +  if (r == 0 || errno != ENOSYS)
> > +    {
> > +      if (! in_time_t_range (trem64.tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      if (remaining)
> > +        *remaining = valid_timespec64_to_timespec (trem64);
> > +    }
> > +
> > +  return r;
> >  }
> > +#endif
> > +
> >  hidden_def (__nanosleep)
> >  weak_alias (__nanosleep, nanosleep)
> > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > index d6442bf4f7f..1a6c6c0a48a 100644
> > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > @@ -17,13 +17,74 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > +#include <kernel-features.h>
> >  #include <sysdep-cancel.h>
> >  #include <not-cancel.h>
> >
> > +int
> > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time,
> > +                             struct __timespec64 *remaining)
> > +{
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +# ifndef __NR_clock_nanosleep_time64
> > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > +# endif
> > +  return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> > +                              requested_time, remaining);
> > +#else
> > +# ifdef __NR_clock_nanosleep_time64
> > +  long int ret_64;
> > +
> > +  ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0,
> > +                                requested_time, remaining);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    return ret_64;
> > +# endif /* __NR_clock_nanosleep_time64 */
> > +  int ret;
> > +  struct timespec ts32, tr32;
> > +
> > +  if (! in_time_t_range (requested_time->tv_sec))
> > +    {
> > +      __set_errno (EOVERFLOW);
> > +      return -1;
> > +    }
> > +
> > +  ts32 = valid_timespec64_to_timespec (*requested_time);
> > +  ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    *remaining = valid_timespec_to_timespec64 (tr32);
> > +
> > +  return ret;
> > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > +}
> > +
> > +#if __TIMESIZE != 64
> >  int
> >  __nanosleep_nocancel (const struct timespec *requested_time,
> > -                   struct timespec *remaining)
> > +                      struct timespec *remaining)
> >  {
> > -  return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining);
> > +  int ret;
> > +  struct __timespec64 treq64, trem64;
> > +
> > +  treq64 = valid_timespec_to_timespec64 (*requested_time);
> > +  ret = __nanosleep_nocancel_time64 (&treq64, &trem64);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    {
> > +      if (! in_time_t_range (trem64.tv_sec))
> > +        {
> > +          __set_errno (EOVERFLOW);
> > +          return -1;
> > +        }
> > +
> > +      if (remaining)
> > +        *remaining = valid_timespec64_to_timespec (trem64);
> > +    }
> > +
> > +  return ret;
> >  }
> > +#endif
> > +
> >  hidden_def (__nanosleep_nocancel)
> >