From 5f3b532c8d3d9492907aca4cbd93dd2e6baf8add Mon Sep 17 00:00:00 2001 From: Liu Zhi Fu Date: Wed, 7 Nov 2018 10:52:33 +0800 Subject: [PATCH] lwip: fix crash caused by sys_mbox_free Fix lwip crashed bug caused by sys_mbox_free() --- components/lwip/lwip | 2 +- .../lwip/port/esp32/freertos/sys_arch.c | 136 ++++++++---------- .../lwip/port/esp32/include/arch/sys_arch.h | 3 +- 3 files changed, 61 insertions(+), 80 deletions(-) diff --git a/components/lwip/lwip b/components/lwip/lwip index 7e636c196..046fadde0 160000 --- a/components/lwip/lwip +++ b/components/lwip/lwip @@ -1 +1 @@ -Subproject commit 7e636c196a1a8d9dc5a6968cef28d3a6b7058129 +Subproject commit 046fadde072b5fca94bea84c16cce5ecbfd6948e diff --git a/components/lwip/port/esp32/freertos/sys_arch.c b/components/lwip/port/esp32/freertos/sys_arch.c index 07830c35a..4270f957b 100644 --- a/components/lwip/port/esp32/freertos/sys_arch.c +++ b/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){ - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("fail to new *mbox->lock\n")); - vQueueDelete((*mbox)->os_mbox); - free(*mbox); - return ERR_MEM; - } +#if ESP_THREAD_SAFE + (*mbox)->owner = NULL; +#endif - (*mbox)->alive = true; - - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("new *mbox ok mbox=%p os_mbox=%p mbox_lock=%p\n", *mbox, (*mbox)->os_mbox, (*mbox)->lock)); + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("new *mbox ok mbox=%p os_mbox=%p\n", *mbox, (*mbox)->os_mbox)); return ERR_OK; } @@ -289,42 +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) { - if (pdTRUE == xQueueReceive((*mbox)->os_mbox, &(*msg), timeout / portTICK_PERIOD_MS)) { - EndTime = xTaskGetTickCount(); - Elapsed = (EndTime - StartTime) * portTICK_PERIOD_MS; - - if (Elapsed == 0) { - Elapsed = 1; - } - - 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; - } - } + if (timeout == 0) { + timeout = portMAX_DELAY; + } else { + timeout = timeout / portTICK_PERIOD_MS; + } + *msg = NULL; + if (pdTRUE == xQueueReceive((*mbox)->os_mbox, &(*msg), timeout)) { EndTime = xTaskGetTickCount(); Elapsed = (EndTime - StartTime) * portTICK_PERIOD_MS; @@ -333,10 +303,10 @@ sys_arch_mbox_fetch(sys_mbox_t *mbox, void **msg, u32_t timeout) } ulReturn = Elapsed; + } else { // timed out blocking for message + ulReturn = SYS_ARCH_TIMEOUT; } - 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 -#define PER_POLL_DELAY 20 - uint16_t count = 0; - bool post_null = true; + uint32_t mbox_message_num = 0; - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: set alive false\n")); - (*mbox)->alive = false; + if ( (NULL == mbox) || (NULL == *mbox) ) { + 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; - } + mbox_message_num = uxQueueMessagesWaiting((*mbox)->os_mbox); - if (post_null){ - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: post null to mbox\n")); - if (sys_mbox_trypost( mbox, NULL) != ERR_OK){ - ESP_STATS_DROP_INC(esp.free_mbox_post_fail); - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: post null mbox fail\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) { + /* Should never be here because post a message to empty mbox should always be successful */ + ESP_LOGW(TAG, "WARNING: failed to post NULL msg to mbox\n"); } else { - post_null = false; - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sys_mbox_free: post null mbox ok\n")); + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("mbox free: post null successfully\n")); } } - - if (count == (MAX_POLL_CNT-1)){ - ESP_LOGW(TAG, "WARNING: mbox %p had a consumer who never unblocked. Leaking!\n", (*mbox)->os_mbox); + (*mbox)->owner = NULL; + } else { + if (mbox_message_num > 1) { + ESP_LOGW(TAG, "WARNING: mbox has %d message, potential memory leaking\n", mbox_message_num); } - sys_delay_ms(PER_POLL_DELAY); + + if (mbox_message_num > 0) { + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("mbox free: reset mbox queue\n")); + xQueueReset((*mbox)->os_mbox); + } + + /* For recvmbox or acceptmbox, free them in netconn_free() when all sockets' API are returned */ + vQueueDelete((*mbox)->os_mbox); + free(*mbox); } - - 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"); - } - +#else vQueueDelete((*mbox)->os_mbox); - sys_mutex_free(&(*mbox)->lock); free(*mbox); +#endif *mbox = NULL; } diff --git a/components/lwip/port/esp32/include/arch/sys_arch.h b/components/lwip/port/esp32/include/arch/sys_arch.h index b13d7d1fe..bfa2ad6ca 100644 --- a/components/lwip/port/esp32/include/arch/sys_arch.h +++ b/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; - uint8_t alive; + void *owner; }* sys_mbox_t;