This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable
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