Re: [PATCH v4] linux: Fix ancillary 64-bit time timestamp conversion (BZ #28349, BZ #28350)

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

> The __convert_scm_timestamps() only updates the control message last
> pointer for SOL_SOCKET type, so if the message control buffer contains
> multiple ancillary message types the converted timestamp one might
> overwrite a valid message.

s/()/ function/?

> The test check if the extra ancillary space is correctly handled

typo: the test check[s]

> by recvmsg/recvmmsg, where if there is no extra space for the 64-bit
> time_t converted message the control buffer should be marked with
> MSG_TRUNC.  It also check if recvmsg/recvmmsg handle correctly multiple
> ancillary data.

typo: MSG_[C]TRUNC

(I think this MSG_CTRUNC result is a remaining bug we need to fix
separately for time32 mode.)

> diff --git a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> index 00c934c413..36976c276f 100644
> --- a/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> +++ b/sysdeps/unix/sysv/linux/convert_scm_timestamps.c
> @@ -54,6 +54,8 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>         cmsg != NULL;
>         cmsg = CMSG_NXTHDR (msg, cmsg))
>      {
> +      last = cmsg;
> +
>        if (cmsg->cmsg_level != SOL_SOCKET)
>  	continue;
>  
> @@ -75,8 +77,6 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>  	  tvts[1] = tmp[1];
>  	  break;
>  	}
> -
> -      last = cmsg;
>      }
>  
>    if (last == NULL || type == 0)

I think the last == NULL check is now redundant.  It's probably clearer
to remove it.

> @@ -88,10 +88,11 @@ __convert_scm_timestamps (struct msghdr *msg, socklen_t msgsize)
>        return;
>      }

(The necessary length check is not visible in the patch, but it is
there.)

> +  /* Zero memory for the new cmsghdr, required by CMSG_NXTHDR.  */

This is specifically about the cmsg_len field, right?  Maybe mention
this in the comment.

> +  memset (msg->msg_control + msg->msg_controllen, 0,
> +	  CMSG_SPACE (sizeof tvts));
>    msg->msg_controllen += CMSG_SPACE (sizeof tvts);
> -  cmsg = CMSG_NXTHDR(msg, last);
> -  if (cmsg == NULL)
> -    return;
> +  cmsg = CMSG_NXTHDR (msg, last);
>    cmsg->cmsg_level = SOL_SOCKET;
>    cmsg->cmsg_type = type;
>    cmsg->cmsg_len = CMSG_LEN (sizeof tvts);

CMSG_NXTHDR cannot be NULL anymore because of the previous length check
and cmsg_len set to zero.  Okay.

> diff --git a/sysdeps/unix/sysv/linux/tst-socket-timestamp.c b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> new file mode 100644
> index 0000000000..3854c46bad
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-socket-timestamp.c
> @@ -0,0 +1,344 @@

> +/* Some extra space added for ancillary data, it might be used to convert
> +   32-bit timestamp to 64-bit for _TIME_BITS=64.  */
> +enum { slack_max_size = 64 };
> +static const int slack[] = { 0, 4, 8, 16, 32, slack_max_size };

I was worried whether 4 is okay here, but we read the cmsg buffer via
memcpy, so we always have sufficient alignment.

> +
> +static bool support_64_timestamp;
> +/* AF_INET socket and address used to send and receive data.  */
> +static int srv;
> +static struct sockaddr_in srv_addr;

I think the comment should mention receiving only.

> +
> +static int
> +do_sendto (const struct sockaddr_in *addr, int nmsgs)
> +{
> +  int s = xsocket (AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> +  xconnect (s, (const struct sockaddr *) addr, sizeof (*addr));
> +
> +  for (int i = 0; i < nmsgs; i++)
> +    xsendto (s, &i, sizeof (i), 0, (const struct sockaddr *) addr,
> +	     sizeof (*addr));
> +
> +  return 0;
> +}

Missing xclose (s).

> +static void
> +do_recvmsg_slack_ancillary (bool use_multi_call, int s, void *cmsg,
> +			    size_t slack, size_t tsize, int exp_msg)
> +{
> +  int msg;

(This is actually the payload.)

> +  /* If there is not timestamp in the ancilliary data, recvmsg should set
> +     the flag inidcating it.  */

typo: in[di]cating

> +  if (exp_timestamp && !timestamp)
> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);

Shouldn't this be MSG_CTRUNC?  I expect this is actually dead code.

> +static void
> +do_test_slack_space (void)
> +{
> +  /* Setup the ancillary data buffer with an extra page with PROT_NONE to
> +     check the possible timestamp conversion on some systems.  */
> +  struct support_next_to_fault nf =
> +    support_next_to_fault_allocate (slack_max_size);
> +  void *msgbuf = nf.buffer + slack_max_size;
> +
> +  /* Enable the timestamp using struct timeval precision.  */
> +  {
> +    int r = setsockopt (srv, SOL_SOCKET, SO_TIMESTAMP, &(int){1},
> +			sizeof (int));
> +    TEST_VERIFY_EXIT (r != -1);
> +  }
> +  /* Check recvmsg.  */
> +  do_sendto (&srv_addr, array_length (slack));
> +  for (int s = 0; s < array_length (slack); s++) {

style: { should be on its own line (repeated below).

> +/* Check if the converted 64-bit timestamp is correctly appended when there
> +   are multiple ancillary messages.  */
> +static void
> +do_recvmsg_multiple_ancillary (bool use_multi_call, int s, void *cmsg,
> +			       size_t cmsgsize, int exp_msg)
> +{

> +  /* If there is no timestamp in the ancillary data, recvmsg should set
> +     the flag to indicate it.  */
> +  if (!timestamp)
> +    TEST_VERIFY (mmhdr.msg_hdr.msg_flags & MSG_TRUNC);

MSG_CTRUNC? (see above)

Thanks,
Florian