[lwip-users] Optimistic error handling in lwIP

[Zschocke, Florian]

I have noticed a quite optimistic error handling in the lwIP sources. While
developing for embedded systems, our systems aren't the most tiny ones so
that every byte would count. I am wondering if such optimistic programming
with lax error checking is wanted in order to keep the code small.

The current example that I have problems with is the assumption that
sys_mbox_fetch() will always successfully return with a fetched message.
Check the netconn_recv() function for an example:

struct netbuf *
netconn_recv(struct netconn *conn)
{
  struct api_msg *msg;
  struct netbuf *buf;
  struct pbuf *p;
  u16_t len;
    
  if(conn == NULL) {
    return NULL;
  }
  
  if(conn->recvmbox == SYS_MBOX_NULL) {
    conn->err = ERR_CONN;
    return NULL;
  }

  if(conn->err != ERR_OK) {
    return NULL;
  }

  if(conn->type == NETCONN_TCP) {

/* TCP code here */

  } else {
    sys_mbox_fetch(conn->recvmbox, (void **)&buf);
    conn->recv_avail -= buf->p->tot_len;
    /* Register event with callback */
    if (conn->callback)
        (*conn->callback)(conn, NETCONN_EVT_RCVMINUS, buf->p->tot_len);
  }

  return buf;
}

Apparently there was some desire for error checking at the beginning of the
function. But in the UDP part the (IMHO optimistic) assumption is made that
sys_mbox_fetch() always returns successfully with buf now pointing to a
valid netbuf. On some (e.g. our) systems that may not be true. Due to some
arbitrary event some interrupt may have occured causing the
sys_arch_mbox_fetch() function to return without having fetched a message.
This will lead to the line after sys_mbox_fetch() to dereference an invalid
pointer which most often leads to a crash.

In this case simply changing the lwIP code to check if (buf != NULL) would
already help. But more generally, I would like to propose to be wary with
void functions. The better choice might be to have sys_mbox_fetch() return
an error value and make ample use of errno throughout the lwIP code. 

The same holds true with the xxx_init() functions. All of them are void
functions. But it could well be that some intialisation fails, e.g. due to
lack of free resources (I am thinking about tcpip_init() here). I think it
would be better if they were declared to return an error code, too, so that
error checking can be done if need be.

Anyhows, I'd like to get a discussion going about this. I'm not quite sure
who is currently "in charge" or actively developing lwIP and who to appeal
to.

Florian.