Re: [PATCH v3 05/24] linux: Add fallback for 64-bit time_t SO_TIMESTAMP{NS}

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

>> See:
>> 
>>   ruby: FTBFS with test suite failure (glibc 2.34 related)
>>   <https://bugzilla.redhat.com/show_bug.cgi?id=1975144>
>
> Do we have a more contained testcase? I am trying to trigger using the ruby
> example it is kind hard to no make it use the system libraries.
>
> I am trying to create a testcase with different cmsghdr sizes, but at least 
> on i686 I can't really reproduce the issue (I am also running on 5.11
> kernel).

The ruby test case needs “set disable-randomization off” in GDB and even
then passes sporadically.  Without it, the test always succeeds for some
reason.

With the GCC 11 compiled binaries in Fedora, we fault in an isolated
error path (null pointer store followed by ud2 instruction).  I haven't
had time to look at the disassembly and GIMPLE dumps to see what is
going on.

>> The disassembly suggests that GCC has detected some undefined behavior.
>> 
>> This looks like a related bug:
>> 
>>   __cmsg_nxthdr in cmsg_nxthdr.c (CMSG_NXTHDR) has undefined behavior when setting up ancillary data
>>   <https://sourceware.org/bugzilla/show_bug.cgi?id=13500>
>> 
>> I believe you cannot use CMSG_NXTHDR to append data in this way.
>
> I think I am getting luck here because the example provided does pass,
> even when tested against valgrind, asan, and ubsan (using gcc 10).

Hmm.  I think it depends on previous buffer contents.

>> The other question is why this code is running at all.  Doing this
>> complex conversion for a 32-bit applications doing a 32-bit function
>> call on a kernel which supports 32-bit system calls does not make much
>> sense to me.
>
> Mainly because kernel does not provide a 64-bit recvmsg, different than
> recvmmg.  So for 64-bit time_t calls we need to do the conversion,
> although it won't help much if the caller does not provide a buffer large
> enough.

Yes, but the Fedora build does not use 64-bit time_t, so this conversion
is kind of pointless there.

I see that we didn't add a __recvmsg_time64 entrypoint (and similar
entrypoints for the other cases).  I think that's a mistake, we should
have those to future-proof things (similar for ioctl and fcntl—are there
any other multiplexers?).

> I don't think we can improve it much by adding a 64-bit symbol: the
> underlying syscall will be the same we don't prior hand which
> SO_TIMESTAMP value were used to setup the timer (32-bit or 64-bit
> one).

Yes, but old 32-bit binaries can avoid running the new code if we have
separate entrypoint.

> Revising the code I found one issue with __convert_scm_timestamps,
> where the memcpy might indeed being accessing invalid memory:
>
> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> index d75a4618dd..2c61267fec 100644
> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> @@ -44,7 +44,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>       'struct __kernel_sock_timeval' while for SO_TIMESTAMPNS_NEW is a
>       'struct __kernel_timespec'.  In either case it is two uint64_t
>       members.  */
> -  uint64_t tvts[2];
> +  int64_t tvts[2];
> +  int32_t tmp;
>  
>    struct cmsghdr *cmsg, *last = NULL;
>    int type = 0;
> @@ -69,7 +70,10 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>  
>         /* fallthrough  */
>         common:
> -         memcpy (tvts, CMSG_DATA (cmsg), sizeof (tvts));
> +         memcpy (&tmp, CMSG_DATA (cmsg), sizeof (tmp));
> +         tvts[0] = tmp;
> +         memcpy (&tmp, CMSG_DATA (cmsg) + sizeof (tmp), sizeof (tmp));
> +         tvts[1] = tmp;
>           break;
>         }

Sorry, I can't quite wrap my head around this right now.

Thanks,
Florian