vquic.c HAVE_SENDMSG recvmsg_packets missing msg_iov reset

[piru]

I did this

It seems that vquic.c HAVE_SENDMSG recvmsg_packets fails to reset the msg_iov.iov_base and msg_iov.iov_len back to buf values when the buffer is been fully filled (that is inficated by msg_iov.iov_len becoming 0):

curl/lib/vquic/vquic.c

Line 497 in 23bed34

while((nread = recvmsg(qctx->sockfd, &msg, 0)) == -1 &&

In this case the code will call recvmsg with iov_len of 0. Depending on the specific of the operating system network stack, this can lead to 0 being returned. If 0 is returned the loop will never terminate as pkts can't advance.

I believe the correct thing to do is to reset the iov_base and iov_len when the whole buffer has been consumed, something like this:

diff --git a/lib/vquic/vquic.c b/lib/vquic/vquic.c
index c1963b473..dedb4e891 100644
--- a/lib/vquic/vquic.c
+++ b/lib/vquic/vquic.c
@@ -494,6 +494,10 @@ static CURLcode recvmsg_packets(struct Curl_cfilter *cf,
     msg.msg_name = &remote_addr;
     msg.msg_namelen = sizeof(remote_addr);
     msg.msg_controllen = sizeof(msg_ctrl);
+    if(msg_iov.iov_len == 0) {
+      msg_iov.iov_base = buf;
+      msg_iov.iov_len = (int)sizeof(buf);
+    }
     while((nread = recvmsg(qctx->sockfd, &msg, 0)) == -1 &&
           SOCKERRNO == SOCKEINTR)
       ;

This seems to work for me, at least, but I am not 100% is this is what should be done here. Note that this codepath is not used on platforms that have HAVE_SENDMMSG.

Bonus idea: Perhaps this and similar fallback codepaths should be exercised by testsuite by explicitly disabling configure flags?

I expected the following

recvmsg_packets not getting stuck in a busyloop.

curl/libcurl version

curl 8.13.0

operating system

MorphOS (network stack is a bsd variant that has recvmsg returning 0 when total iov_len is 0)

[piru]

On further analysis even this proposed fix is wrong. Since the offset always start from the beginning of buf the correct thing seems to be to reset the msg_iov always before the recvmsg call:

diff --git a/lib/vquic/vquic.c b/lib/vquic/vquic.c
index c1963b473..5f99bf74e 100644
--- a/lib/vquic/vquic.c
+++ b/lib/vquic/vquic.c
@@ -481,9 +481,6 @@ static CURLcode recvmsg_packets(struct Curl_cfilter *cf,
  size_t pktlen;
  size_t offset, to;

-  msg_iov.iov_base = buf;
-  msg_iov.iov_len = (int)sizeof(buf);
-
  memset(&msg, 0, sizeof(msg));
  msg.msg_iov = &msg_iov;
  msg.msg_iovlen = 1;
@@ -494,6 +491,8 @@ static CURLcode recvmsg_packets(struct Curl_cfilter *cf,
    msg.msg_name = &remote_addr;
    msg.msg_namelen = sizeof(remote_addr);
    msg.msg_controllen = sizeof(msg_ctrl);
+    msg_iov.iov_base = buf;
+    msg_iov.iov_len = (int)sizeof(buf);
    while((nread = recvmsg(qctx->sockfd, &msg, 0)) == -1 &&
          SOCKERRNO == SOCKEINTR)
      ;

But then the question can be raised: What's the point of using recvmsg in the first place? What does it offer over the classic recvfrom code?

[icing]

So, you say there are OS that modify msg_iov.iov_lenduring the call? Oof.

[piru]

That's a good point. I will actually verify that this is really happening.

[piru]

@icing Here is the relevant part: https://github.com/NetBSD/src/blob/62ada091aa7963b3d5e7817f98a9bbd9ad38f949/sys/kern/subr_copy.c#L103

So for example in recvmsg the iov are "filled" in order. Once the specific iov is full, it will have iov_len of 0 (and iov_base will point to the end of the iov buffer).

[icing]

Arg. ok. Thanks for finding this.

[icing]

Proposed fix in #17131.

[piru]

Actually, netbsd is not to blame after all. They do copy the structure from userland to kernel memory properly. After the internal processing, the msghdr is copied back, but iovec are not (obviously, as they should not be touched).

So how come this is an issue in code that was derived from netbsd?

When AmiTCP/IP was created in 1993, the peeps working on it took networking code from netbsd 0.9. Since AmigaOS has no separation between user and kernel memory, in their bout of optimisation, they removed some buffer copying and missed the distinction of iovec not being copied back to userland. Instead, they made the recvmsg/sendmsg pass the user-passed struct msghdr as-is to the internal engine doing the processing. This leads to the iovec being modified when it should not be.

Oops.

Somehow, this went unnoticed for 32 years. Thanks, libcurl, for dragging this ugly bit to the surface!