Fixed bug #27871: Calling tcp_abort() in recv callback can lead to accessing unallocated memory. As a consequence, ERR_ABRT means the application has called tcp_abort()!

CHANGELOG

@@ -53,6 +53,11 @@ HISTORY
 
   ++ Bugfixes:
 
+  2010-01-27: Simon Goldschmidt
+  * tcp.h, tcp.c, tcp_in.c: Fixed bug #27871: Calling tcp_abort() in recv
+    callback can lead to accessing unallocated memory. As a consequence,
+    ERR_ABRT means the application has called tcp_abort()!
+
   2010-01-25: Simon Goldschmidt
   * snmp_structs.h, msg_in.c: Partly fixed bug #22070 (MIB_OBJECT_WRITE_ONLY
     not implemented in SNMP): write-only or not-accessible are still

doc/rawapi.txt

@@ -278,6 +278,11 @@ again when the connection has been idle for a while.
   Aborts the connection by sending a RST (reset) segment to the remote
   host. The pcb is deallocated. This function never fails.
 
+  ATTENTION: When calling this from one of the TCP callbacks, make
+  sure you always return ERR_ABRT (and never return ERR_ABRT otherwise
+  or you will risk accessing deallocated memory or memory leaks!
+
+
 If a connection is aborted because of an error, the application is
 alerted of this event by the err callback. Errors that might abort a
 connection are when there is a shortage of memory. The callback

src/core/tcp.c

@@ -760,20 +760,21 @@ tcp_slowtmr(void)
       memp_free(MEMP_TCP_PCB, pcb);
       pcb = pcb2;
     } else {
+      /* get the 'next' element now and work with 'prev' below (in case of abort) */
+      prev = pcb;
+      pcb = pcb->next;
 
       /* We check if we should poll the connection. */
-      ++pcb->polltmr;
-      if (pcb->polltmr >= pcb->pollinterval) {
-        pcb->polltmr = 0;
+      ++prev->polltmr;
+      if (prev->polltmr >= prev->pollinterval) {
+        prev->polltmr = 0;
         LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: polling application\n"));
-        TCP_EVENT_POLL(pcb, err);
+        TCP_EVENT_POLL(prev, err);
+        /* if err == ERR_ABRT, 'prev' is already deallocated */
         if (err == ERR_OK) {
-          tcp_output(pcb);
+          tcp_output(prev);
         }
       }
-      
-      prev = pcb;
-      pcb = pcb->next;
     }
   }
 
@@ -823,9 +824,10 @@ tcp_slowtmr(void)
 void
 tcp_fasttmr(void)
 {
-  struct tcp_pcb *pcb;
+  struct tcp_pcb *pcb = tcp_active_pcbs;
 
-  for(pcb = tcp_active_pcbs; pcb != NULL; pcb = pcb->next) {
+  while(pcb != NULL) {
+    struct tcp_pcb *next = pcb->next;
     /* If there is data which was previously "refused" by upper layer */
     if (pcb->refused_data != NULL) {
       /* Notify again application with data previously received. */
@@ -834,16 +836,21 @@ tcp_fasttmr(void)
       TCP_EVENT_RECV(pcb, pcb->refused_data, ERR_OK, err);
       if (err == ERR_OK) {
         pcb->refused_data = NULL;
+      } else if (err == ERR_ABRT) {
+        /* if err == ERR_ABRT, 'pcb' is already deallocated */
+        pcb = NULL;
       }
     }
 
     /* send delayed ACKs */  
-    if (pcb->flags & TF_ACK_DELAY) {
+    if (pcb && (pcb->flags & TF_ACK_DELAY)) {
       LWIP_DEBUGF(TCP_DEBUG, ("tcp_fasttmr: delayed ACK\n"));
       tcp_ack_now(pcb);
       tcp_output(pcb);
       pcb->flags &= ~(TF_ACK_DELAY | TF_ACK_NOW);
     }
+
+    pcb = next;
   }
 }
 

src/core/tcp_in.c

@@ -280,6 +280,7 @@ tcp_input(struct pbuf *p, struct netif *inp)
       if (err == ERR_OK) {
         pcb->refused_data = NULL;
       } else {
+        /* if err == ERR_ABRT, 'pcb' is already deallocated */
         /* drop incoming packets, because pcb is "full" */
         LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_input: drop incoming packets, because pcb is \"full\"\n"));
         TCP_STATS_INC(tcp.drop);
@@ -313,8 +314,11 @@ tcp_input(struct pbuf *p, struct netif *inp)
            now. */
         if (pcb->acked > 0) {
           TCP_EVENT_SENT(pcb, pcb->acked, err);
+          if (err == ERR_ABRT) {
+            goto aborted;
+          }
         }
-      
+
         if (recv_data != NULL) {
           if(flags & TCP_PSH) {
             recv_data->flags |= PBUF_FLAG_PUSH;
@@ -322,6 +326,9 @@ tcp_input(struct pbuf *p, struct netif *inp)
 
           /* Notify application that data has been received. */
           TCP_EVENT_RECV(pcb, recv_data, ERR_OK, err);
+          if (err == ERR_ABRT) {
+            goto aborted;
+          }
 
           /* If the upper layer can't receive this data, store it */
           if (err != ERR_OK) {
@@ -334,6 +341,9 @@ tcp_input(struct pbuf *p, struct netif *inp)
            function with a NULL buffer to indicate EOF. */
         if (recv_flags & TF_GOT_FIN) {
           TCP_EVENT_RECV(pcb, NULL, ERR_OK, err);
+          if (err == ERR_ABRT) {
+            goto aborted;
+          }
         }
 
         tcp_input_pcb = NULL;
@@ -346,6 +356,9 @@ tcp_input(struct pbuf *p, struct netif *inp)
 #endif /* TCP_INPUT_DEBUG */
       }
     }
+    /* Jump target if pcb has been aborted in a callback (by calling tcp_abort()).
+       Below this line, 'pcb' may not be dereferenced! */
+aborted:
     tcp_input_pcb = NULL;
 
 
@@ -616,6 +629,9 @@ tcp_process(struct tcp_pcb *pcb)
       /* Call the user specified function to call when sucessfully
        * connected. */
       TCP_EVENT_CONNECTED(pcb, ERR_OK, err);
+      if (err == ERR_ABRT) {
+        return ERR_ABRT;
+      }
       tcp_ack_now(pcb);
     }
     /* received ACK? possibly a half-open connection */
@@ -640,7 +656,10 @@ tcp_process(struct tcp_pcb *pcb)
         if (err != ERR_OK) {
           /* If the accept function returns with an error, we abort
            * the connection. */
-          tcp_abort(pcb);
+          /* Already aborted? */
+          if (err != ERR_ABRT) {
+            tcp_abort(pcb);
+          }
           return ERR_ABRT;
         }
         old_cwnd = pcb->cwnd;

src/include/lwip/tcp.h

@@ -54,7 +54,9 @@ struct tcp_pcb;
  *
  * @param arg Additional argument to pass to the callback function (@see tcp_arg())
  * @param newpcb The new connection pcb
- * @param err An error code if there has been an error accepting
+ * @param err An error code if there has been an error accepting.
+ *            Only return ERR_ABRT if you have called tcp_abort from within the
+ *            callback function!
  */
 typedef err_t (*tcp_accept_fn)(void *arg, struct tcp_pcb *newpcb, err_t err);
 
@@ -65,6 +67,8 @@ typedef err_t (*tcp_accept_fn)(void *arg, struct tcp_pcb *newpcb, err_t err);
  * @param tpcb The connection pcb which received data
  * @param p The received data (or NULL when the connection has been closed!)
  * @param err An error code if there has been an error receiving
+ *            Only return ERR_ABRT if you have called tcp_abort from within the
+ *            callback function!
  */
 typedef err_t (*tcp_recv_fn)(void *arg, struct tcp_pcb *tpcb,
                              struct pbuf *p, err_t err);
@@ -77,6 +81,8 @@ typedef err_t (*tcp_recv_fn)(void *arg, struct tcp_pcb *tpcb,
  * @param tpcb The connection pcb for which data has been acknowledged
  * @param len The amount of bytes acknowledged
  * @return ERR_OK: try to send some data by calling tcp_output
+ *            Only return ERR_ABRT if you have called tcp_abort from within the
+ *            callback function!
  */
 typedef err_t (*tcp_sent_fn)(void *arg, struct tcp_pcb *tpcb,
                               u16_t len);
@@ -87,6 +93,8 @@ typedef err_t (*tcp_sent_fn)(void *arg, struct tcp_pcb *tpcb,
  * @param arg Additional argument to pass to the callback function (@see tcp_arg())
  * @param tpcb tcp pcb
  * @return ERR_OK: try to send some data by calling tcp_output
+ *            Only return ERR_ABRT if you have called tcp_abort from within the
+ *            callback function!
  */
 typedef err_t (*tcp_poll_fn)(void *arg, struct tcp_pcb *tpcb);
 
@@ -109,6 +117,8 @@ typedef void  (*tcp_err_fn)(void *arg, err_t err);
  * @param arg Additional argument to pass to the callback function (@see tcp_arg())
  * @param tpcb The connection pcb which is connected
  * @param err An unused error code, always ERR_OK currently ;-) TODO!
+ *            Only return ERR_ABRT if you have called tcp_abort from within the
+ *            callback function!
  *
  * @note When a connection attempt fails, the error callback is currently called!
  */