Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

[Joseph Myers <joseph at codesourcery dot com>]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com