[lwip-devel] [bug #20900] Potential crash error problem with netconn_peer & netconn_addr

[Frédéric Bernon]

URL:
  <http://savannah.nongnu.org/bugs/?20900>

                 Summary: Potential crash error problem with netconn_peer &
netconn_addr
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: fbernon
            Submitted on: samedi 25.08.2007 à 15:39
                Category: None
                Severity: 3 - Normal
              Item Group: Crash Error
                  Status: None
                 Privacy: Public
             Assigned to: fbernon
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: 1.3.0

    _______________________________________________________

Details:

I think there is a potential crash error problem with netconn_peer &
netconn_addr

netconn_peer can return :
- ERR_ARG if (conn != NULL) 
- ERR_CONN if (conn->pcb.tcp == NULL)

The first is already checked in lwip_accept (the netconn_accept result), the
second could appear if between the time we accept the "newconn", a err_tcp
appears and set to NULL conn->pcb.tcp. Ok, it could be possible, but, since
the err_tcp is processed in a different thread (tcpip_thread) than the
application thread, this can even appear just after the check in
netconn_peer:

  case NETCONN_TCP:
    if (conn->pcb.tcp == NULL)
      return ERR_CONN;
    <err_tcp is call here in tcpip_thread, and set pcb.tcp to NULL>
    *addr = (conn->pcb.tcp->remote_ip); <<<CRASH
    *port = conn->pcb.tcp->remote_port;
    break;

So, in a general way, it's not safe to call the current netconn_peer with a
NETCONN_TCP. Same for netconn_addr (more, there is not the "if (conn->pcb.tcp
== NULL) return ERR_CONN;"). At socket layer, it give "same" problems with
lwip_getsockname (which call netconn_addr), and lwip_getpeername (which call
netconn_peer). For these both functions, to be safe-thread, I think we should
go in tcpip_thread context to get these values by adding do_getpeername &
do_getaddrname (but could use the current api_msg_msg's "bc" union for that).
At runtime, "lwip_accept" would be slower, and I think that
lwip_getsockname/netconn_addr and lwip_getpeername/netconn_peer are now often
called (but perhaps I'm wrong).

There is also this check in netconn_recv: "if (conn->pcb.tcp->state ==
LISTEN) {". For this one, we could set conn->state to NETCONN_ACCEPT (from
netconn_state enum) in do_listen, and replace this check by "if (conn->state
== NETCONN_ACCEPT). Better, since NETCONN_ACCEPT is not currently used in the
source code, I would rename it NETCONN_LISTEN. Last, I think this check is
done to process application error (call a recv on a socket in listen mode), we
should replace it by something like this :

LWIP_ERROR("netconn_recv: conn is in listen mode", (conn->state !=
NETCONN_ACCEPT), conn->err = ERR_CONN; return NULL;);





    _______________________________________________________

Reply to this item at:

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

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