tcp: changed accept handling to be done internally: the application does not have to call tcp_accepted() any more. Instead, when delaying accept (e.g. sockets do), call tcp_backlog_delayed()/tcp_backlog_accepted() (fixes bug #46696)

CHANGELOG

@@ -302,6 +302,11 @@ HISTORY
 
   ++ Bugfixes:
 
+  2016-03-22: Simon Goldschmidt
+  * tcp: changed accept handling to be done internally: the application does not
+    have to call tcp_accepted() any more. Instead, when delaying accept (e.g. sockets
+    do), call tcp_backlog_delayed()/tcp_backlog_accepted() (fixes bug #46696)
+
   2016-03-22: Simon Goldschmidt
   * dns.c: ignore dns response parsing errors, only abort resolving for correct
     responses or error responses from correct server (bug #47459)

src/api/api_lib.c

@@ -386,9 +386,9 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn)
 #if TCP_LISTEN_BACKLOG
   /* Let the stack know that we have accepted the connection. */
   API_MSG_VAR_ALLOC_DONTFAIL(msg);
-  API_MSG_VAR_REF(msg).conn = conn;
+  API_MSG_VAR_REF(msg).conn = newconn;
   /* don't care for the return value of lwip_netconn_do_recv */
-  netconn_apimsg(lwip_netconn_do_recv, &API_MSG_VAR_REF(msg));
+  netconn_apimsg(lwip_netconn_do_accepted, &API_MSG_VAR_REF(msg));
   API_MSG_VAR_FREE(msg);
 #endif /* TCP_LISTEN_BACKLOG */
 

src/api/api_msg.c

@@ -485,6 +485,9 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err)
      to the application thread */
   newconn->last_err = err;
 
+  /* handle backlog counter */
+  tcp_backlog_delayed(newpcb);
+
   if (sys_mbox_trypost(&conn->acceptmbox, newconn) != ERR_OK) {
     /* When returning != ERR_OK, the pcb is aborted in tcp_process(),
        so do nothing here! */
@@ -501,6 +504,8 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err)
     sys_mbox_free(&newconn->recvmbox);
     sys_mbox_set_invalid(&newconn->recvmbox);
     netconn_free(newconn);
+    /* handle backlog counter */
+    tcp_backlog_accepted(newpcb);
     return ERR_MEM;
   } else {
     /* Register event with callback */
@@ -761,8 +766,8 @@ netconn_drain(struct netconn *conn)
       struct netconn *newconn = (struct netconn *)mem;
       /* Only tcp pcbs have an acceptmbox, so no need to check conn->type */
       /* pcb might be set to NULL already by err_tcp() */
-      if (conn->pcb.tcp != NULL) {
+      if (newconn->pcb.tcp != NULL) {
-        tcp_accepted(conn->pcb.tcp);
+        tcp_backlog_accepted(newconn->pcb.tcp);
       }
       /* drain recvmbox */
       netconn_drain(newconn);
@@ -1430,24 +1435,38 @@ lwip_netconn_do_recv(void *m)
   msg->err = ERR_OK;
   if (msg->conn->pcb.tcp != NULL) {
     if (NETCONNTYPE_GROUP(msg->conn->type) == NETCONN_TCP) {
-#if TCP_LISTEN_BACKLOG
+      u32_t remaining = msg->msg.r.len;
-      if (msg->conn->pcb.tcp->state == LISTEN) {
+      do {
-        tcp_accepted(msg->conn->pcb.tcp);
+        u16_t recved = (remaining > 0xffff) ? 0xffff : (u16_t)remaining;
-      } else
+        tcp_recved(msg->conn->pcb.tcp, recved);
-#endif /* TCP_LISTEN_BACKLOG */
+        remaining -= recved;
-      {
+      } while (remaining != 0);
-        u32_t remaining = msg->msg.r.len;
-        do {
-          u16_t recved = (remaining > 0xffff) ? 0xffff : (u16_t)remaining;
-          tcp_recved(msg->conn->pcb.tcp, recved);
-          remaining -= recved;
-        } while (remaining != 0);
-      }
     }
   }
   TCPIP_APIMSG_ACK(msg);
 }
 
+#if TCP_LISTEN_BACKLOG
+/** Indicate that a TCP pcb has been accepted
+ * Called from netconn_accept
+ *
+ * @param msg the api_msg_msg pointing to the connection
+ */
+void
+lwip_netconn_do_accepted(void *m)
+{
+  struct api_msg *msg = (struct api_msg*)m;
+
+  msg->err = ERR_OK;
+  if (msg->conn->pcb.tcp != NULL) {
+    if (NETCONNTYPE_GROUP(msg->conn->type) == NETCONN_TCP) {
+      tcp_backlog_accepted(msg->conn->pcb.tcp);
+    }
+  }
+  TCPIP_APIMSG_ACK(msg);
+}
+#endif /* TCP_LISTEN_BACKLOG */
+
 /**
  * See if more data needs to be written from a previous call to netconn_write.
  * Called initially from lwip_netconn_do_write. If the first call can't send all data

src/apps/httpd/httpd.c

@@ -2286,12 +2286,10 @@ static err_t
 http_accept(void *arg, struct tcp_pcb *pcb, err_t err)
 {
   struct http_state *hs;
-  struct tcp_pcb_listen *lpcb = (struct tcp_pcb_listen*)arg;
+  LWIP_UNUSED_ARG(arg);
   LWIP_UNUSED_ARG(err);
   LWIP_DEBUGF(HTTPD_DEBUG, ("http_accept %p / %p\n", (void*)pcb, arg));
 
-  /* Decrease the listen backlog counter */
-  tcp_accepted(lpcb);
   /* Set priority */
   tcp_setprio(pcb, HTTPD_TCP_PRIO);
 
@@ -2343,8 +2341,7 @@ httpd_init(void)
   LWIP_ASSERT("httpd_init: tcp_bind failed", err == ERR_OK);
   pcb = tcp_listen(pcb);
   LWIP_ASSERT("httpd_init: tcp_listen failed", pcb != NULL);
-  /* initialize callback arg and accept callback */
+  /* initialize accept callback */
-  tcp_arg(pcb, pcb);
   tcp_accept(pcb, http_accept);
 }
 

src/core/tcp.c

@@ -147,6 +147,78 @@ tcp_tmr(void)
   }
 }
 
+#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG
+/** Called when a listen pcb is closed. Iterates one pcb list and removes the
+ * closed listener pcb from pcb->listener if matching.
+ */
+static void
+tcp_remove_listener(struct tcp_pcb *list, struct tcp_pcb_listen *lpcb)
+{
+   struct tcp_pcb *pcb;
+   for (pcb = list; pcb != NULL; pcb = pcb->next) {
+      if (pcb->listener == lpcb) {
+         pcb->listener = NULL;
+      }
+   }
+}
+#endif
+
+/** Called when a listen pcb is closed. Iterates all pcb lists and removes the
+ * closed listener pcb from pcb->listener if matching.
+ */
+static void
+tcp_listen_closed(struct tcp_pcb *pcb)
+{
+#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG
+  size_t i;
+  LWIP_ASSERT("pcb != NULL", pcb != NULL);
+  LWIP_ASSERT("pcb->state == LISTEN", pcb->state == LISTEN);
+  for (i = 1; i < LWIP_ARRAYSIZE(tcp_pcb_lists); i++) {
+    tcp_remove_listener(*tcp_pcb_lists[i], (struct tcp_pcb_listen*)pcb);
+  }
+#endif
+  LWIP_UNUSED_ARG(pcb);
+}
+
+#if TCP_LISTEN_BACKLOG
+/** Delay accepting a connection in respect to the listen backlog:
+ * the number of outstanding connections is increased until
+ * tcp_backlog_accepted() is called.
+ *
+ * ATTENTION: the caller is responsible for calling tcp_backlog_accepted()
+ * or else the backlog feature will get out of sync!
+ *
+ * @param pcb the connection pcb which is not fully accepted yet
+ */
+void
+tcp_backlog_delayed(struct tcp_pcb* pcb)
+{
+  LWIP_ASSERT("pcb != NULL", pcb != NULL);
+  if (pcb->listener != NULL) {
+    pcb->listener->accepts_pending++;
+    LWIP_ASSERT("accepts_pending != 0", pcb->listener->accepts_pending != 0);
+  }
+}
+
+/** A delayed-accept a connection is accepted (or closed/aborted): decreases
+ * the number of outstanding connections after calling tcp_backlog_delayed().
+ *
+ * ATTENTION: the caller is responsible for calling tcp_backlog_accepted()
+ * or else the backlog feature will get out of sync!
+ *
+ * @param pcb the connection pcb which is now fully accepted (or closed/aborted)
+ */
+void
+tcp_backlog_accepted(struct tcp_pcb* pcb)
+{
+  LWIP_ASSERT("pcb != NULL", pcb != NULL);
+  if (pcb->listener != NULL) {
+    LWIP_ASSERT("accepts_pending != 0", pcb->listener->accepts_pending != 0);
+    pcb->listener->accepts_pending--;
+  }
+}
+#endif /* TCP_LISTEN_BACKLOG */
+
 /**
  * Closes the TX side of a connection held by the PCB.
  * For tcp_close(), a RST is sent if the application didn't receive all data
@@ -217,6 +289,7 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data)
   case LISTEN:
     err = ERR_OK;
     tcp_pcb_remove(&tcp_listen_pcbs.pcbs, pcb);
+    tcp_listen_closed(pcb);
     memp_free(MEMP_TCP_PCB_LISTEN, pcb);
     pcb = NULL;
     break;
@@ -230,6 +303,7 @@ tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data)
   case SYN_RCVD:
     err = tcp_send_fin(pcb);
     if (err == ERR_OK) {
+      tcp_backlog_accepted(pcb);
       MIB2_STATS_INC(mib2.tcpattemptfails);
       pcb->state = FIN_WAIT_1;
     }
@@ -1586,9 +1660,10 @@ tcp_err(struct tcp_pcb *pcb, tcp_err_fn err)
 void
 tcp_accept(struct tcp_pcb *pcb, tcp_accept_fn accept)
 {
-  /* This function is allowed to be called for both listen pcbs and
+  if (pcb->state == LISTEN) {
-     connection pcbs. */
+    struct tcp_pcb_listen *lpcb = (struct tcp_pcb_listen*)pcb;
-  pcb->accept = accept;
+    lpcb->accept = accept;
+  }
 }
 #endif /* LWIP_CALLBACK_API */
 
@@ -1628,22 +1703,9 @@ tcp_pcb_purge(struct tcp_pcb *pcb)
 
 #if TCP_LISTEN_BACKLOG
     if (pcb->state == SYN_RCVD) {
-      /* Need to find the corresponding listen_pcb and decrease its accepts_pending */
+      tcp_backlog_accepted(pcb);
-      struct tcp_pcb_listen *lpcb;
+      /* prevent executing this again: */
-      LWIP_ASSERT("tcp_pcb_purge: pcb->state == SYN_RCVD but tcp_listen_pcbs is NULL",
+      pcb->listener = NULL;
-        tcp_listen_pcbs.listen_pcbs != NULL);
-      for (lpcb = tcp_listen_pcbs.listen_pcbs; lpcb != NULL; lpcb = lpcb->next) {
-        if ((lpcb->local_port == pcb->local_port) &&
-            (IP_IS_V6_VAL(pcb->local_ip) == IP_IS_V6_VAL(lpcb->local_ip)) &&
-            (ip_addr_isany(&lpcb->local_ip) ||
-             ip_addr_cmp(&pcb->local_ip, &lpcb->local_ip))) {
-            /* port and address of the listen pcb match the timed-out pcb */
-            LWIP_ASSERT("tcp_pcb_purge: listen pcb does not have accepts pending",
-              lpcb->accepts_pending > 0);
-            lpcb->accepts_pending--;
-            break;
-          }
-      }
     }
 #endif /* TCP_LISTEN_BACKLOG */
 

src/core/tcp_in.c

@@ -579,9 +579,9 @@ tcp_listen_input(struct tcp_pcb_listen *pcb)
     npcb->rcv_ann_right_edge = npcb->rcv_nxt;
     npcb->snd_wl1 = seqno - 1;/* initialise to seqno-1 to force window update */
     npcb->callback_arg = pcb->callback_arg;
-#if LWIP_CALLBACK_API
+#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG
-    npcb->accept = pcb->accept;
+    npcb->listener = pcb;
-#endif /* LWIP_CALLBACK_API */
+#endif /* LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG */
     /* inherit socket options */
     npcb->so_options = pcb->so_options & SOF_INHERITED;
     /* Register the new PCB so that we can begin receiving segments
@@ -784,10 +784,18 @@ tcp_process(struct tcp_pcb *pcb)
         pcb->state = ESTABLISHED;
         LWIP_DEBUGF(TCP_DEBUG, ("TCP connection established %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
 #if LWIP_CALLBACK_API
-        LWIP_ASSERT("pcb->accept != NULL", pcb->accept != NULL);
+        LWIP_ASSERT("pcb->listener->accept != NULL",
+          (pcb->listener == NULL) || (pcb->listener->accept != NULL));
+        if (pcb->listener == NULL) {
+          /* listen pcb might be closed by now */
+          err = ERR_VAL;
+        } else
 #endif
-        /* Call the accept function. */
+        {
-        TCP_EVENT_ACCEPT(pcb, ERR_OK, err);
+          tcp_backlog_accepted(pcb);
+          /* Call the accept function. */
+          TCP_EVENT_ACCEPT(pcb, ERR_OK, err);
+        }
         if (err != ERR_OK) {
           /* If the accept function returns with an error, we abort
            * the connection. */

src/include/lwip/priv/api_msg.h

@@ -184,6 +184,9 @@ void lwip_netconn_do_disconnect      (void *m);
 void lwip_netconn_do_listen          (void *m);
 void lwip_netconn_do_send            (void *m);
 void lwip_netconn_do_recv            (void *m);
+#if TCP_LISTEN_BACKLOG
+void lwip_netconn_do_accepted        (void *m);
+#endif /* TCP_LISTEN_BACKLOG */
 void lwip_netconn_do_write           (void *m);
 void lwip_netconn_do_getaddr         (void *m);
 void lwip_netconn_do_close           (void *m);

src/include/lwip/priv/tcp_priv.h

@@ -216,11 +216,11 @@ PACK_STRUCT_END
 
 #else /* LWIP_EVENT_API */
 
-#define TCP_EVENT_ACCEPT(pcb,err,ret)                          \
+#define TCP_EVENT_ACCEPT(pcb,err,ret)                                   \
-  do {                                                         \
+  do {                                                                  \
-    if((pcb)->accept != NULL)                                  \
+    if(((pcb)->listener != NULL) && ((pcb)->listener->accept != NULL))  \
-      (ret) = (pcb)->accept((pcb)->callback_arg,(pcb),(err));  \
+      (ret) = (pcb)->listener->accept((pcb)->callback_arg,(pcb),(err)); \
-    else (ret) = ERR_ARG;                                      \
+    else (ret) = ERR_ARG;                                               \
   } while (0)
 
 #define TCP_EVENT_SENT(pcb,space,ret)                          \

src/include/lwip/tcp.h

@@ -155,34 +155,38 @@ enum tcp_state {
   TIME_WAIT   = 10
 };
 
-#if LWIP_CALLBACK_API
-  /* Function to call when a listener has been connected.
-   * @param arg user-supplied argument (tcp_pcb.callback_arg)
-   * @param pcb a new tcp_pcb that now is connected
-   * @param err an error argument (TODO: that is current always ERR_OK?)
-   * @return ERR_OK: accept the new connection,
-   *                 any other err_t aborts the new connection
-   */
-#define DEF_ACCEPT_CALLBACK  tcp_accept_fn accept;
-#else /* LWIP_CALLBACK_API */
-#define DEF_ACCEPT_CALLBACK
-#endif /* LWIP_CALLBACK_API */
-
 /**
  * members common to struct tcp_pcb and struct tcp_listen_pcb
  */
 #define TCP_PCB_COMMON(type) \
   type *next; /* for the linked list */ \
   void *callback_arg; \
-  /* the accept callback for listen- and normal pcbs, if LWIP_CALLBACK_API */ \
-  DEF_ACCEPT_CALLBACK \
   enum tcp_state state; /* TCP state */ \
   u8_t prio; \
   /* ports are in host byte order */ \
   u16_t local_port
 
 
-/* the TCP protocol control block */
+/** the TCP protocol control block for listening pcbs */
+struct tcp_pcb_listen {
+/** Common members of all PCB types */
+  IP_PCB;
+/** Protocol specific PCB members */
+  TCP_PCB_COMMON(struct tcp_pcb_listen);
+
+#if LWIP_CALLBACK_API
+  /* Function to call when a listener has been connected. */
+  tcp_accept_fn accept;
+#endif /* LWIP_CALLBACK_API */
+
+#if TCP_LISTEN_BACKLOG
+  u8_t backlog;
+  u8_t accepts_pending;
+#endif /* TCP_LISTEN_BACKLOG */
+};
+
+
+/** the TCP protocol control block */
 struct tcp_pcb {
 /** common PCB members */
   IP_PCB;
@@ -268,6 +272,10 @@ struct tcp_pcb {
 
   struct pbuf *refused_data; /* Data previously received but not yet taken by upper layer */
 
+#if LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG
+  struct tcp_pcb_listen* listener;
+#endif /* LWIP_CALLBACK_API || TCP_LISTEN_BACKLOG */
+
 #if LWIP_CALLBACK_API
   /* Function to be called when more send buffer space is available. */
   tcp_sent_fn sent;
@@ -307,18 +315,6 @@ struct tcp_pcb {
 #endif
 };
 
-struct tcp_pcb_listen {
-/* Common members of all PCB types */
-  IP_PCB;
-/* Protocol specific PCB members */
-  TCP_PCB_COMMON(struct tcp_pcb_listen);
-
-#if TCP_LISTEN_BACKLOG
-  u8_t backlog;
-  u8_t accepts_pending;
-#endif /* TCP_LISTEN_BACKLOG */
-};
-
 #if LWIP_EVENT_API
 
 enum lwip_event {
@@ -353,21 +349,21 @@ void             tcp_err     (struct tcp_pcb *pcb, tcp_err_fn err);
 #define          tcp_sndbuf(pcb)          (TCPWND16((pcb)->snd_buf))
 #define          tcp_sndqueuelen(pcb)     ((pcb)->snd_queuelen)
 #define          tcp_nagle_disable(pcb)   ((pcb)->flags |= TF_NODELAY)
-#define          tcp_nagle_enable(pcb)    ((pcb)->flags = (tcpflags_t)((pcb)->flags & ~TF_NODELAY))
+#define          tcp_nagle_enable(pcb)    ((pcb)->flags = (u16_t)((pcb)->flags & ~TF_NODELAY))
 #define          tcp_nagle_disabled(pcb)  (((pcb)->flags & TF_NODELAY) != 0)
 
 #if TCP_LISTEN_BACKLOG
-#define          tcp_accepted(pcb) do { \
-  LWIP_ASSERT("pcb->state == LISTEN (called for wrong pcb?)", pcb->state == LISTEN); \
-  (((struct tcp_pcb_listen *)(pcb))->accepts_pending--); } while(0)
 #define          tcp_backlog_set(pcb, new_backlog) do { \
   LWIP_ASSERT("pcb->state == LISTEN (called for wrong pcb?)", (pcb)->state == LISTEN); \
   ((struct tcp_pcb_listen *)(pcb))->backlog = ((new_backlog) ? (new_backlog) : 1); } while(0)
+void             tcp_backlog_delayed(struct tcp_pcb* pcb);
+void             tcp_backlog_accepted(struct tcp_pcb* pcb);
 #else  /* TCP_LISTEN_BACKLOG */
-#define          tcp_accepted(pcb) LWIP_ASSERT("pcb->state == LISTEN (called for wrong pcb?)", \
-                                               (pcb)->state == LISTEN)
 #define          tcp_backlog_set(pcb, new_backlog)
+#define          tcp_backlog_delayed(pcb)
+#define          tcp_backlog_accepted(pcb)
 #endif /* TCP_LISTEN_BACKLOG */
+//#define          tcp_accepted(pcb) /* compatibility define, not needed any more */
 
 void             tcp_recved  (struct tcp_pcb *pcb, u16_t len);
 err_t            tcp_bind    (struct tcp_pcb *pcb, const ip_addr_t *ipaddr,