Re: [PATCH v2 05/25] linux: Add fallback for 64-bit time_t SO_{RCV, SND}TIMEO

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 21/05/2021 15:37, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 20/05/2021 03:44, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> +#ifndef __ASSUME_TIME64_SYSCALLS
>>>> +static int
>>>> +getsockopt32 (int fd, int level, int optname, void *optval,
>>>> +	      socklen_t *len)
>>>> +{
>>>> +  int r = -1;
>>>> +
>>>> +  if (level != SOL_SOCKET)
>>>> +    return r;
>>>> +
>>>> +  switch (optname)
>>>> +    {
>>>> +    case COMPAT_SO_RCVTIMEO_NEW:
>>>> +    case COMPAT_SO_SNDTIMEO_NEW:
>>>> +      {
>>>> +        if (*len < sizeof (struct __timeval64))
>>>> +	  {
>>>> +	    __set_errno (EINVAL);
>>>> +	    break;
>>>> +	  }
>>>> +
>>>> +	if (optname == COMPAT_SO_RCVTIMEO_NEW)
>>>> +	  optname = COMPAT_SO_RCVTIMEO_OLD;
>>>> +	if (optname == COMPAT_SO_SNDTIMEO_NEW)
>>>> +	  optname = COMPAT_SO_SNDTIMEO_OLD;
>>>> +
>>>> +	struct __timeval32 tv32;
>>>> +	r = getsockopt_syscall (fd, level, optname, &tv32,
>>>> +				(socklen_t[]) { sizeof tv32 });
>>>> +	if (r < 0)
>>>> +	  break;
>>>> +	struct __timeval64 *tv64 = (struct __timeval64 *) optval;
>>>> +	*tv64 = valid_timeval32_to_timeval64 (tv32);
>>>> +	*len = sizeof (*tv64);
>>>> +      }
>>>
>>> I think POSIX requires checking the incoming *len value and truncating
>>> the output.
>>
>> The kernel will fail with EINVAL if the input argument is smaller than
>> the expected one, so I think as QoI the truncation here should mean
>> to return a timeout value clamped to 32 bits (instead of a truncated
>> memory).
> 
> Wow, that EINVAL behavior is not POSIX-compliant at all.
> 
> If the kernel behaves that way, I guess glibc should as well.

In fact, checking again the source it does seem that silently truncated 
the result:

net/core/sock.c

1420         case SO_RCVTIMEO_OLD:
1421         case SO_RCVTIMEO_NEW:
1422                 lv = sock_get_timeout(sk->sk_rcvtimeo, &v, SO_RCVTIMEO_OLD == optname);
1423                 break;
[...]
1624         if (len > lv)
1625                 len = lv;
1626         if (copy_to_user(optval, &v, len))
1627                 return -EFAULT;

So I think maybe we should do the same here, not sure if this is really
an improvement over compatibility.