[PATCH v6 2/5] linux: Use long time_t __getitimer/__setitimer

[Alistair Francis]

On Mon, Mar 30, 2020 at 9:34 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 30/03/2020 13:17, Alistair Francis wrote:
> > On Mon, Mar 30, 2020 at 7:49 AM Adhemerval Zanella via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >>
> >>
> >> On 29/03/2020 19:19, Stepan Golosunov wrote:
> >>> 29.03.2020 в 11:17:17 -0700 Alistair Francis написал:
> >>>> On Sun, Mar 29, 2020 at 2:59 AM Stepan Golosunov <stepan@golosunov.pp.ru> wrote:
> >>>>>
> >>>>> 28.03.2020 в 08:22:46 -0700 Alistair Francis написал:
> >>>>>> --- /dev/null
> >>>>>> +++ b/sysdeps/unix/sysv/linux/getitimer.c
> >>>>>
> >>>>>> +int
> >>>>>> +__getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value)
> >>>>>> +{
> >>>>>> +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
> >>>>>> +  return INLINE_SYSCALL_CALL (getitimer, which, curr_value);
> >>>>>> +#else
> >>>>>> +  struct __itimerval32 curr_value_32;
> >>>>>> +
> >>>>>> +  if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1)
> >>>>>> +    return -1;
> >>>>>> +
> >>>>>> +  curr_value->it_interval
> >>>>>> +    = valid_timeval32_to_timeval64 (curr_value_32.it_interval);
> >>>>>> +  curr_value->it_value
> >>>>>> +    = valid_timeval32_to_timeval64 (curr_value_32.it_value);
> >>>>>> +  return 0;
> >>>>>> +#endif
> >>>>>> +}
> >>>>>> +
> >>>>>> +#if __TIMESIZE != 64
> >>>>>> +libc_hidden_def (__getitimer64)
> >>>>>> +int
> >>>>>> +__getitimer (__itimer_which_t which, struct itimerval *curr_value)
> >>>>>> +{
> >>>>>> +  struct __itimerval64 val64;
> >>>>>> +
> >>>>>> +  val64.it_interval
> >>>>>> +    = valid_timeval_to_timeval64 (curr_value->it_interval);
> >>>>>> +  val64.it_value
> >>>>>> +    = valid_timeval_to_timeval64 (curr_value->it_value);
> >>>>>> +
> >>>>>> +  return __getitimer64 (which, &val64);
> >>>>>> +}
> >>>>>> +#endif
> >>>>>> +weak_alias (__getitimer, getitimer)
> >>>>>
> >>>>> __getitimer treats curr_value as input-only variable, while it's an
> >>>>> output-only one in __getitimer64.  This won't work.
> >>>>
> >>>> I'm not sure what you mean here, can you please elaborate?
> >>>
> >>> __getitimer (…, &curr_value) will never write to curr_value (or do
> >>> anything useful with it at all); while
> >>> __getitimer64 (…, &curr_value) will write to curr_value as expected.
> >>>
> >>> Conversion in __getitimer shold be in opposite direction and after
> >>> __getitimer64 call.
> >>>
> >>
> >> Indeed, it should be:
> >>
> >>   int
> >>   __getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value)
> >>   {
> >>     [...]
> >>   }
> >>
> >>   #if __TIMESIZE != 64
> >>   libc_hidden_def (__getitimer64)
> >>
> >>   int
> >>   __getitimer (__itimer_which_t which, struct itimerval *curr_value)
> >>   {
> >>     struct __itimerval64 val64;
> >>     if (__getitimer64 (which, &val64) != 0)
> >>       return -1;
> >>
> >>     curr_value->it_interval
> >>       = valid_timeval64_to_timeval (val64->it_interval);
> >>     curr_value->it_value
> >>       = valid_timeval64_to_timeval (val64->it_value);
> >>
> >>     return 0;
> >>   }
> >>   #endif
> >
> > I changed it to this (which is very similar to above). Good catch on that.
> >
> > #if __TIMESIZE != 64
> > libc_hidden_def (__getitimer64)
> > int
> > __getitimer (__itimer_which_t which, struct itimerval *curr_value)
> > {
> >   struct __itimerval64 val64;
> >   int ret = __getitimer64 (which, &val64);
>
> There is no need to actually check the return value, POSIX states it
> return -1 on failure (and __getitimer64 will set errno accordingly).
>
> >
> >   if (ret == 0 && curr_value)
>
> Again there is no need to check if 'curr_value', neither POSIX or
> kernels add such constraints (and kernel does return EFAULT in
> such case).

We don't pass curr_value to the kernel, so this will just segfault if
curr_value is invalid.

I will remove the check though.

Alistair

>
> >     {
> >       curr_value->it_interval
> >         = valid_timeval64_to_timeval (val64.it_interval);
> >       curr_value->it_value
> >         = valid_timeval64_to_timeval (val64.it_value);
> >     }
> >
> >   return ret;
>