[lwip-devel] TCP_REG/RMV macros bugs?

[Tomas Hruby]

Hi all,

I have some doubts about the correctness of handling the tcp pcb
lists using the TCP_REG/RMV macros. I am using only the raw API of an
older version (1.4.0rc1) but the relevant code hasn't changed much. I
may use it incorrectly in my socket layer. The problem I observed is
that lwip may create a loop in the tcp_tw_pcbs list which then results
in infinite (long) looping in tcp_slowtmr().

When I enabled lists debugging, it quickly failed on an assert as the
macros then check whether a pcb is already in a list or not. It seems
to me that if a connection is closed by the remote end and the local
end closes the connection later on (here I may not use the API
correctly) using tcp_close(), the connection is already in the list
and gets added again. It does not seem that using tcp_close(0 is
obiously incorrect and it seems to me that somewhere should be a check
whether a tcp is already in a list, do not add it again. Below is my
take on fixing it.

I added tracking in which list a pcb is and if adding a pcb in the
same list again, let is succeed without modifying the list. This
should not be necessary if the rest of the code would guarantee that
his cannot happen whic is seems it does not.

A related problem is when removing a pcb from a list. The macro does
not check if the pcb is really in the list. It may be in a different
one. In such a case, the ->next is set to NULL and it may break a
wrong list. Again, the wrapping code should make sure that it calls it
on the right list only, however, it does not seem it is true. At least
in my version of lwip.

I am not sure how easy is it to reproduce it. I was running openssh
server and some much simpler apps with this stack for 2-3 years and it
only popped up when I started using lighttpd and thus the pattern
changed.

If you would agree with me, the loop checking whether a pcb is on a
list would be redundant.

Cheer,

Tomas

diff --git a/src/include/lwip/tcp.h b/src/include/lwip/tcp.h
index add77e8..1322973 100644
--- a/src/include/lwip/tcp.h
+++ b/src/include/lwip/tcp.h
@@ -156,6 +156,7 @@ enum tcp_state {
  * members common to struct tcp_pcb and struct tcp_listen_pcb
  */
 #define TCP_PCB_COMMON(type) \
+  type **list; /* in what list is the tcb */ \
   type *next; /* for the linked list */ \
   void *callback_arg; \
   /* the accept callback for listen- and normal pcbs, if LWIP_CALLBACK_API */ \
diff --git a/src/include/lwip/tcp_impl.h b/src/include/lwip/tcp_impl.h
index 2afc20d..32924d6 100644
--- a/src/include/lwip/tcp_impl.h
+++ b/src/include/lwip/tcp_impl.h
@@ -336,6 +336,8 @@ extern struct tcp_pcb *tcp_tmp_pcb;      /* Only used for 
temporary storage. */
 #endif
 #if TCP_DEBUG_PCB_LISTS
 #define TCP_REG(pcbs, npcb) do {\
+                            if ((npcb)->list == (pcbs)) break; \
+                            LWIP_ASSERT("TCP_REG: already registered 
elsewhere", (npcb)->list == NULL); \
                             LWIP_DEBUGF(TCP_DEBUG, ("TCP_REG %p local port 
%d\n", (npcb), (npcb)->local_port)); \
                             for(tcp_tmp_pcb = *(pcbs); \
           tcp_tmp_pcb != NULL; \
@@ -347,9 +349,11 @@ extern struct tcp_pcb *tcp_tmp_pcb;      /* Only used for 
temporary storage. */
                             LWIP_ASSERT("TCP_REG: npcb->next != npcb", 
(npcb)->next != (npcb)); \
                             *(pcbs) = (npcb); \
                             LWIP_ASSERT("TCP_RMV: tcp_pcbs sane", 
tcp_pcbs_sane()); \
+                            (npcb)->list = (pcbs); \
               tcp_timer_needed(); \
                             } while(0)
 #define TCP_RMV(pcbs, npcb) do { \
+                            if ((npcb)->list != (pcbs)) break; \
                             LWIP_ASSERT("TCP_RMV: pcbs != NULL", *(pcbs) != 
NULL); \
                             LWIP_DEBUGF(TCP_DEBUG, ("TCP_RMV: removing %p from 
%p\n", (npcb), *(pcbs))); \
                             if(*(pcbs) == (npcb)) { \
@@ -361,6 +365,7 @@ extern struct tcp_pcb *tcp_tmp_pcb;      /* Only used for 
temporary storage. */
                                } \
                             } \
                             (npcb)->next = NULL; \
+                            (npcb)->list = NULL; \
                             LWIP_ASSERT("TCP_RMV: tcp_pcbs sane", 
tcp_pcbs_sane()); \
                             LWIP_DEBUGF(TCP_DEBUG, ("TCP_RMV: removed %p from 
%p\n", (npcb), *(pcbs))); \
                             } while(0)
@@ -369,13 +374,16 @@ extern struct tcp_pcb *tcp_tmp_pcb;      /* Only used for 
temporary storage. */
 
 #define TCP_REG(pcbs, npcb)                        \
   do {                                             \
+    if ((npcb)->list == (pcbs)) break;             \
     (npcb)->next = *pcbs;                          \
     *(pcbs) = (npcb);                              \
+    (npcb)->list = (pcbs);                         \
     tcp_timer_needed();                            \
   } while (0)
 
 #define TCP_RMV(pcbs, npcb)                        \
   do {                                             \
+    if ((npcb)->list != (pcbs)) break;             \
     if(*(pcbs) == (npcb)) {                        \
       (*(pcbs)) = (*pcbs)->next;                   \
     }                                              \
@@ -390,6 +398,7 @@ extern struct tcp_pcb *tcp_tmp_pcb;      /* Only used for 
temporary storage. */
       }                                            \
     }                                              \
     (npcb)->next = NULL;                           \
+    (npcb)->list = NULL;                           \
   } while(0)
 
 #endif /* LWIP_DEBUG */