Merge branch 'bugfix/tw27096_fix_lwip_mbox_free_crash' into 'master'

fix the crash bug caused by sys_mbox_free

See merge request idf/esp-idf!3654

components/lwip/lwip

@@ -1 +1 @@
-Subproject commit 7e636c196a1a8d9dc5a6968cef28d3a6b7058129
+Subproject commit 046fadde072b5fca94bea84c16cce5ecbfd6948e

components/lwip/port/esp32/freertos/sys_arch.c

@@ -222,16 +222,11 @@ sys_mbox_new(sys_mbox_t *mbox, int size)
     return ERR_MEM;
   }
 
-  if (sys_mutex_new(&((*mbox)->lock)) != ERR_OK){
+#if ESP_THREAD_SAFE
-    LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("fail to new *mbox->lock\n"));
+  (*mbox)->owner = NULL;
-    vQueueDelete((*mbox)->os_mbox);
+#endif
-    free(*mbox);
-    return ERR_MEM;
-  }
 
-  (*mbox)->alive = true;
+  LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("new *mbox ok mbox=%p os_mbox=%p\n", *mbox, (*mbox)->os_mbox));
-
-  LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("new *mbox ok mbox=%p os_mbox=%p mbox_lock=%p\n", *mbox, (*mbox)->os_mbox, (*mbox)->lock));
   return ERR_OK;
 }
 
@@ -289,14 +284,17 @@ sys_arch_mbox_fetch(sys_mbox_t *mbox, void **msg, u32_t timeout)
 
   if (*mbox == NULL){
     *msg = NULL;
-    LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_arch_mbox_fetch: null mbox\n"));
     return -1;
   }
 
-  sys_mutex_lock(&(*mbox)->lock);
+  if (timeout == 0) {
+    timeout = portMAX_DELAY;
+  } else {
+    timeout = timeout / portTICK_PERIOD_MS;
+  }
 
-  if (timeout != 0) {
+  *msg = NULL;
-    if (pdTRUE == xQueueReceive((*mbox)->os_mbox, &(*msg), timeout / portTICK_PERIOD_MS)) {
+  if (pdTRUE == xQueueReceive((*mbox)->os_mbox, &(*msg), timeout)) {
     EndTime = xTaskGetTickCount();
     Elapsed = (EndTime - StartTime) * portTICK_PERIOD_MS;
 
@@ -306,36 +304,8 @@ sys_arch_mbox_fetch(sys_mbox_t *mbox, void **msg, u32_t timeout)
 
     ulReturn = Elapsed;
   } else { // timed out blocking for message
-      *msg = NULL;
     ulReturn = SYS_ARCH_TIMEOUT;
   }
-  } else { // block forever for a message.
-    
-    while (1){
-      LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_arch_mbox_fetch: fetch mbox=%p os_mbox=%p lock=%p\n", mbox, (*mbox)->os_mbox, (*mbox)->lock));
-      if (pdTRUE == xQueueReceive((*mbox)->os_mbox, &(*msg), portMAX_DELAY)){
-        LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_arch_mbox_fetch:mbox rx msg=%p\n", (*msg)));
-        break;
-      }
-
-      if ((*mbox)->alive == false){
-        LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_arch_mbox_fetch:mbox not alive\n"));
-        *msg = NULL;
-        break;
-      }
-    }
-
-    EndTime = xTaskGetTickCount();
-    Elapsed = (EndTime - StartTime) * portTICK_PERIOD_MS;
-
-    if (Elapsed == 0) {
-      Elapsed = 1;
-    }
-
-    ulReturn = Elapsed;
-  }
-
-  sys_mutex_unlock(&(*mbox)->lock);
 
   return ulReturn ; // return time blocked TBD test
 }
@@ -361,6 +331,16 @@ sys_arch_mbox_tryfetch(sys_mbox_t *mbox, void **msg)
 }
 
 /*-----------------------------------------------------------------------------------*/
+
+void
+sys_mbox_set_owner(sys_mbox_t *mbox, void* owner)
+{
+  if (mbox && *mbox) {
+    (*mbox)->owner = owner;
+    LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("set mbox=%p owner=%p", *mbox, owner));
+  }
+}
+
 /*
   Deallocates a mailbox. If there are messages still present in the
   mailbox when the mailbox is deallocated, it is an indication of a
@@ -369,50 +349,52 @@ sys_arch_mbox_tryfetch(sys_mbox_t *mbox, void **msg)
 void
 sys_mbox_free(sys_mbox_t *mbox)
 {
-#define MAX_POLL_CNT 100
+  uint32_t mbox_message_num = 0;
-#define PER_POLL_DELAY 20
-  uint16_t count = 0;
-  bool post_null = true;
 
-  LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: set alive false\n"));
+  if ( (NULL == mbox) || (NULL == *mbox) ) {
-  (*mbox)->alive = false;
+      ESP_LOGW(TAG, "WARNING: free null mbox\n");
-
+      return;
-  while ( count++ < MAX_POLL_CNT ){ //ESP32_WORKAROUND
-    LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free:try lock=%d\n", count));
-    if (!sys_mutex_trylock( &(*mbox)->lock )){
-      LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free:get lock ok %d\n", count));
-      sys_mutex_unlock( &(*mbox)->lock );
-      break;
   }
 
-    if (post_null){
+  mbox_message_num = uxQueueMessagesWaiting((*mbox)->os_mbox);
-      LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: post null to mbox\n"));
+
+  LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("mbox free: mbox=%p os_mbox=%p owner=%p msg_num=%d\n", 
+              *mbox, (*mbox)->os_mbox, (*mbox)->owner, mbox_message_num));
+
+#if ESP_THREAD_SAFE
+  if ((*mbox)->owner) {
+    if (0 == mbox_message_num) {
+      /*
+       * If mbox->owner is not NULL, it indicates the mbox is recvmbox or acceptmbox,
+       * we need to post a NULL message to mbox in case some application tasks are blocked
+       * on this mbox
+       */
       if (sys_mbox_trypost(mbox, NULL) != ERR_OK) {
-        ESP_STATS_DROP_INC(esp.free_mbox_post_fail);
+        /* Should never be here because post a message to empty mbox should always be successful */
-        LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: post null mbox fail\n"));
+        ESP_LOGW(TAG, "WARNING: failed to post NULL msg to mbox\n");
       } else {
-        post_null = false;
+        LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("mbox free: post null successfully\n"));
-        LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: post null mbox ok\n"));
       }
     }
+    (*mbox)->owner = NULL;
+  } else {
+    if (mbox_message_num > 1) {
+      ESP_LOGW(TAG, "WARNING: mbox has %d message, potential memory leaking\n", mbox_message_num);
+    }
 
-    if (count == (MAX_POLL_CNT-1)){
+    if (mbox_message_num > 0) {
-      ESP_LOGW(TAG, "WARNING: mbox %p had a consumer who never unblocked. Leaking!\n", (*mbox)->os_mbox);
+      LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("mbox free: reset mbox queue\n"));
-    }
-    sys_delay_ms(PER_POLL_DELAY);
-  }
-
-  LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free:free mbox\n"));
-
-  if (uxQueueMessagesWaiting((*mbox)->os_mbox)) {
       xQueueReset((*mbox)->os_mbox); 
-    /* Line for breakpoint.  Should never break here! */
-    __asm__ volatile ("nop");
     }
 
+    /* For recvmbox or acceptmbox, free them in netconn_free() when all sockets' API are returned */
     vQueueDelete((*mbox)->os_mbox);
-  sys_mutex_free(&(*mbox)->lock);
     free(*mbox);
+  }
+#else
+  vQueueDelete((*mbox)->os_mbox);
+  free(*mbox);
+#endif
   *mbox = NULL;
 }
 

components/lwip/port/esp32/include/arch/sys_arch.h

@@ -50,8 +50,7 @@ typedef xTaskHandle sys_thread_t;
 
 typedef struct sys_mbox_s {
   xQueueHandle os_mbox;
-  sys_mutex_t  lock;
+  void *owner;
-  uint8_t      alive;
 }* sys_mbox_t;