[lwip-devel] [bug #19162] lwip_sendto: possible to corrupt remote addr/port connection state

[Frédéric Bernon]

Follow-up Comment #12, bug #19162 (project lwip):

Some remarks. In netbuf_ref, pbuf_alloc result is not tested.

void
netbuf_ref(struct netbuf *buf, const void *dataptr, u16_t size)
{
  if (buf->p != NULL) {
    pbuf_free(buf->p);
  }
  buf->p = pbuf_alloc(PBUF_TRANSPORT, 0, PBUF_REF);
  if (buf->p == NULL) { <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
     return;            <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  }                     <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  buf->p->payload = (void*)dataptr;
  buf->p->len = buf->p->tot_len = size;
  buf->ptr = buf->p;
}

When you receive lot of datas on a raw or udp socket, you can easily got a
"not enough memory" on send (same resource pool - MEMP_NETBUF). It will be
better to replace netbuf_new in lwip_send by a local variable :

int
lwip_send(int s, const void *data, int size, unsigned int flags)
{
  struct lwip_socket *sock;
  struct netbuf buf;
  err_t err;

  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_send(%d, data=%p, size=%d,
flags=0x%x)\n", s, data, size, flags));

  sock = get_socket(s);
  if (!sock)
    return -1;

  switch (netconn_type(sock->conn)) {
  case NETCONN_RAW:
  case NETCONN_UDP:
  case NETCONN_UDPLITE:
  case NETCONN_UDPNOCHKSUM:
    /* init a buffer */
    buf.p = buf.ptr = NULL;

    /* make the buffer point to the data that should be sent */
    netbuf_ref(&buf, data, size);
    if (buf.p!=NULL) {
      /* send the data */
      err = netconn_send(sock->conn, &buf);
      /* free packet buffer */
      pbuf_free(buf->p);
    } else {
      err = ERR_MEM;
    }

    break;
  case NETCONN_TCP:
    err = netconn_write(sock->conn, data, size, NETCONN_COPY);
    break;
  default:
    err = ERR_ARG;
    break;
  }
  if (err != ERR_OK) {
    LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_send(%d) err=%d\n", s, err));
    sock_set_errno(sock, err_to_errno(err));
    return -1;
  }

  LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_send(%d) ok size=%d\n", s, size));
  sock_set_errno(sock, 0);
  return size;
}

I propose to replace lwip_read & lwip_recv with define using lwip_recvfrom.
Same about lwip_write on lwip_send.

With lwip_sendto on a connection-oriented socket (TCP), the to and tolen
parameters are ignored, making sendto equivalent to lwip_send. I propose to
handle that at the beginning of lwip_sendto with this line :
if (netconn_type(sock->conn)==NETCONN_TCP) return lwip_send( s, data, size,
flags);

With lwip_send on a message-oriented socket (UDPxx, RAW), the call have to be
process like a lwip_sendto to "default" address (the one connected). I propose
to handle that at the beginning of lwip_send with this line :

if (netconn_type(sock->conn)!=NETCONN_TCP) return lwip_sendto( s, data, size,
flags, NULL, 0); 
We have to upgrade the lwip_sendto assert for that (or pass a real "struct
sockaddr" with 0 in s_addr). I consider is not necessary to handle a
"undefined" netconn_type to avoid recursive calls, because only lwip_socket
can create netconn, and all types are defined.

With these first changes, we can tell that lwip_send is for TCP, and
lwip_sendto for others (UDP & RAW)...

Next step is to set in lwip_sendto fromaddr & fromport with (??) "to"
parameters, and to call netconn_send. No change in netconn_send. In do_send,
we can add this part of lwip_sendto (not like this, but this is the idea):

  /* get the peer if currently connected */
  connected = (netconn_peer(sock->conn, &addr, &port) == ERR_OK);
  remote_addr.addr = ((struct sockaddr_in *)to)->sin_addr.s_addr;
  remote_port = ((struct sockaddr_in *)to)->sin_port;
  netconn_connect(sock->conn, &remote_addr, ntohs(remote_port));

  // raw_send or udp_send
  
  /* reset the remote address and port number of the connection */
  if (connected)
    netconn_connect(sock->conn, &addr, port);
  else
  netconn_disconnect(sock->conn);

......

I have other patchs in the pipe I wait to commit tomorrow, and next, I could
propose a patch file...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?19162>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/