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

[Adhemerval Zanella]

On 30/03/2020 13:27, Alistair Francis wrote:
> 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.

I meant that kernels also does not check if the input 'curr_value' is
NULL prior issue copy_to_user.  This might leads to failure in invalid
code that checks for EFAULT (LTP is one that actually has regression
for it), but as for getrlimit change (to call prlimit{64}) we can't
guarantee that EFAULT is generated.