comment #1:

> From the description below, I'm tempted to change pbuf_clone() to just skip zero-sized pbufs (if anything has been copied yet). I guess that would be enough to fix this?

I think so. Maybe also change pbuf_clone to not ignore pbuf_copy errors? (Free the allocated pbuf and return NULL)

Hi,

I call lwip_sendmsg from LwIP 2.1.0 (release) with an UDP socket like this:

  static int udp_tx(int udp_skt, const void *PAYLOAD, size_t PAYLOAD_LENGTH)
  {
    struct iovec iov[2];
    struct msghdr msg;
    msg.msg_name = &remote_addr; /* global */
    msg.msg_namelen = sizeof(remote_addr);
    msg.msg_iov = iov;
    msg.msg_control = NULL;
    msg.msg_controllen = 0;
    msg.msg_flags = 0;
    iov[0].iov_base = &PROTOCOL_MAGIC;
    iov[0].iov_len = sizeof(PROTOCOL_MAGIC); /* 4 bytes */
    iov[1].iov_base = (void *)PAYLOAD;
    iov[1].iov_len = PAYLOAD_LENGTH;
    msg.msg_iovlen = 2;
    return lwip_sendmsg(udp_skt, &msg, 0);
  }

Usually, the payload is a string of bytes, but sometimes the function
"udp_tx" is called with zero-sized payload (end-of-transmission marker).

In those cases, the UDP packet on wire seems to contain random junk
instead of just whatever should be in the PROTOCOL_MAGIC.

After some debugging, it came to the low-level ethernet driver in my project,
which contains code like this:

  /* Check payload alignment in `p` */
  if ((uintptr_t)p->payload % MEM_ALIGNMENT != 0) {
    /* Create a single pbuf long enough to store the whole chain */
    struct pbuf *q = pbuf_clone(PBUF_RAW, PBUF_RAM, p);
    if (!q) ... error out ...
    p = q;
  } ...
  ... Pass p->payload to hardware ...

The Ethernet hardware apparently needs a contiguous, aligned data to transfer,
and the code tried to provide that.

The problem is that pbuf_clone returns not the expected pbuf containing the
flattened chain given in the "p" argument. The implementation in 2.1.0 (and if
I'm not mistaken, also in master) will return the allocated, partially filled
buffer, ignoring returned error from pbuf_copy!

The pbuf_copy in this case returns the errors because of this code:

    if ((p_from != NULL) && (p_from->len == p_from->tot_len)) {
      /* don't copy more than one packet! */
      LWIP_ERROR("pbuf_copy() does not allow packet queues!",
                 (p_from->next == NULL), return ERR_VAL;);
    }

On its final iterations, having processed Ethernet, IP, UDP, and the magic
from application, it reaches the zero-sized msg_iov item and returns with an
error code, which pbuf_clone mostly ignores (might print a debug message) and
returns new pbuf to the caller, which has no way to know that it is not the
clone it wanted.

Now, arguably "sendmsg" shouldn't have created such an evil chain of pbufs.
On the other hand, pbuf_clone is a public function and is even prepared to
fail (it returns NULL). On yet another hand, pbuf_copy may simply skip over
such buffers.

Right now, pbuf_clone just silently produces partial clones. And sendmsg silently
(under some circumstances, admitted) sends corrupted packets.