lwip: fix mbox thread-safe issue

Fix a mbox free thread-safe issue that can lead to crash in sys_arch_mbox_fetch.

diff --git a/components/lwip/api/api_lib.c b/components/lwip/api/api_lib.c
index 92ea4dced18360417b27dd7dc7b9660db770987f..3dd589b727624ca7987aaac127ccbd88cc21f831 100644 (file)
--- a/components/lwip/api/api_lib.c
+++ b/components/lwip/api/api_lib.c
@@ -128,6 +128,9 @@ netconn_new_with_proto_and_callback(enum netconn_type t, u8_t proto, netconn_cal
       LWIP_ASSERT("conn has no op_completed", sys_sem_valid(&conn->op_completed));
       sys_sem_free(&conn->op_completed);
 #endif /* !LWIP_NETCONN_SEM_PER_THREAD */
+#if ESP_THREAD_SAFE
+      sys_mbox_set_owner(&conn->recvmbox, NULL);
+#endif
       sys_mbox_free(&conn->recvmbox);
       memp_free(MEMP_NETCONN, conn);
       return NULL;

diff --git a/components/lwip/api/api_msg.c b/components/lwip/api/api_msg.c
index 960548f35f8a16f8a2cc8b0ee44e7138019e8d0b..d0c22e69d45ec3e39ac8e3a5ae5078039a8145e8 100644 (file)
--- a/components/lwip/api/api_msg.c
+++ b/components/lwip/api/api_msg.c
@@ -552,6 +552,9 @@ accept_function(void *arg, struct tcp_pcb *newpcb, err_t err)
     tcp_err(pcb, NULL);
     /* remove reference from to the pcb from this netconn */
     newconn->pcb.tcp = NULL;
+#if ESP_THREAD_SAFE
+    sys_mbox_set_owner(&newconn->recvmbox, NULL);
+#endif
     /* no need to drain since we know the recvmbox is empty. */
     sys_mbox_free(&newconn->recvmbox);
     sys_mbox_set_invalid(&newconn->recvmbox);
@@ -711,14 +714,14 @@ netconn_alloc(enum netconn_type t, netconn_callback callback)
 #endif
 
 #if ESP_THREAD_SAFE
-  conn->recvmbox_ref = conn->recvmbox;
   sys_mbox_set_owner(&conn->recvmbox, conn);
-#if LWIP_TCP
-  sys_mbox_set_invalid(&conn->acceptmbox_ref);
-#endif
 #endif
 
 #if LWIP_TCP
+#if ESP_THREAD_SAFE
+  /* Init acceptmbox to NULL because sys_mbox_set_invalid is implemented as empty macro */
+  conn->acceptmbox = NULL;
+#endif
   sys_mbox_set_invalid(&conn->acceptmbox);
 #endif
   conn->state        = NETCONN_NONE;
@@ -761,24 +764,21 @@ void
 netconn_free(struct netconn *conn)
 {
   LWIP_ASSERT("PCB must be deallocated outside this function", conn->pcb.tcp == NULL);
+
+#if !ESP_THREAD_SAFE
   LWIP_ASSERT("recvmbox must be deallocated before calling this function",
     !sys_mbox_valid(&conn->recvmbox));
 #if LWIP_TCP
   LWIP_ASSERT("acceptmbox must be deallocated before calling this function",
     !sys_mbox_valid(&conn->acceptmbox));
 #endif /* LWIP_TCP */
-
-#if ESP_THREAD_SAFE
-  if (conn->recvmbox_ref) {
-    sys_mbox_free(&conn->recvmbox_ref);
-  }
+#else /* !ESP_THREAD_SAFE */
+  sys_mbox_free(&conn->recvmbox);
 
 #if LWIP_TCP
-  if (conn->acceptmbox_ref) {
-    sys_mbox_free(&conn->acceptmbox_ref);
-  }
-#endif
+  sys_mbox_free(&conn->acceptmbox);
 #endif
+#endif /* !ESP_THREAD_SAFE */
 
 #if !LWIP_NETCONN_SEM_PER_THREAD
   sys_sem_free(&conn->op_completed);
@@ -1420,7 +1420,6 @@ lwip_netconn_do_listen(void *m)
               }
               if (msg->err == ERR_OK) {
 #if ESP_THREAD_SAFE
-                msg->conn->acceptmbox_ref = msg->conn->acceptmbox;
                 sys_mbox_set_owner(&msg->conn->acceptmbox, msg->conn);
 #endif
                 msg->conn->state = NETCONN_LISTEN;

diff --git a/components/lwip/include/lwip/lwip/api.h b/components/lwip/include/lwip/lwip/api.h
index fa97091225810f2e10c64d221c75a80e5e86b2a1..5b6a21ecf38c211826ed9ed596b3a915f1725ed4 100644 (file)
--- a/components/lwip/include/lwip/lwip/api.h
+++ b/components/lwip/include/lwip/lwip/api.h
@@ -195,15 +195,6 @@ struct netconn {
       by the application thread */
   sys_mbox_t acceptmbox;
 #endif /* LWIP_TCP */
-
-#if ESP_THREAD_SAFE
-  /** point to the same mbox as recvmbox */
-  sys_mbox_t recvmbox_ref;
-#if LWIP_TCP
-  /** point to the same mbox as acceptmbox */
-  sys_mbox_t acceptmbox_ref;
-#endif
-#endif
   /** only used for socket layer */
 #if LWIP_SOCKET
   int socket;

diff --git a/components/lwip/include/lwip/port/arch/sys_arch.h b/components/lwip/include/lwip/port/arch/sys_arch.h
index f3a0625b8ff984d1ca5a3d2aa6aa4c2c8d74c865..3a3932734310e4c2fa7d9ee9d8e21295643c0b73 100644 (file)
--- a/components/lwip/include/lwip/port/arch/sys_arch.h
+++ b/components/lwip/include/lwip/port/arch/sys_arch.h
@@ -62,7 +62,25 @@ typedef struct sys_mbox_s {
 #endif\r
 \r
 #define sys_mbox_valid( x ) ( ( ( *x ) == NULL) ? pdFALSE : pdTRUE )\r
-#define sys_mbox_set_invalid( x ) ( ( *x ) = NULL )\r
+
+/* Define the sys_mbox_set_invalid() to empty to support lock-free mbox in ESP LWIP.
+ *
+ * The basic idea about the lock-free mbox is that the mbox should always be valid unless
+ * no socket APIs are using the socket and the socket is closed. ESP LWIP achieves this by
+ * following two changes to official LWIP:
+ * 1. Postpone the deallocation of mbox to netconn_free(), in other words, free the mbox when
+ *    no one is using the socket.
+ * 2. Define the sys_mbox_set_invalid() to empty if the mbox is not actually freed.
+
+ * The second change is necessary. Consider a common scenario: the application task calls
+ * recv() to receive packets from the socket, the sys_mbox_valid() returns true. Because there
+ * is no lock for the mbox, the LWIP CORE can call sys_mbox_set_invalid() to set the mbox at
+ * anytime and the thread-safe issue may happen.
+ *
+ * However, if the sys_mbox_set_invalid() is not called after sys_mbox_free(), e.g. in netconn_alloc(),
+ * we need to initialize the mbox to invalid explicitly since sys_mbox_set_invalid() now is empty.
+ */
+#define sys_mbox_set_invalid( x )
 
 #define sys_sem_valid( x ) ( ( ( *x ) == NULL) ? pdFALSE : pdTRUE )\r
 #define sys_sem_set_invalid( x ) ( ( *x ) = NULL )\r

diff --git a/components/lwip/port/freertos/sys_arch.c b/components/lwip/port/freertos/sys_arch.c
index 671a6f92a443d7071654f39806aaa6dfb94f6c71..3067493f9fca817a40db89b55afe462cf9923087 100644 (file)
--- a/components/lwip/port/freertos/sys_arch.c
+++ b/components/lwip/port/freertos/sys_arch.c
@@ -352,7 +352,6 @@ sys_mbox_free(sys_mbox_t *mbox)
   uint32_t mbox_message_num = 0;
 
   if ( (NULL == mbox) || (NULL == *mbox) ) {
-      ESP_LOGW(TAG, "WARNING: free null mbox\n");
       return;
   }
 
@@ -390,12 +389,13 @@ sys_mbox_free(sys_mbox_t *mbox)
     /* For recvmbox or acceptmbox, free them in netconn_free() when all sockets' API are returned */
     vQueueDelete((*mbox)->os_mbox);
     free(*mbox);
+    *mbox = NULL;
   }
 #else
   vQueueDelete((*mbox)->os_mbox);
   free(*mbox);
-#endif
   *mbox = NULL;
+#endif
 }
 
 /*-----------------------------------------------------------------------------------*/