From fcc140c11c2ab89670b0698da68528a49fc7e77d Mon Sep 17 00:00:00 2001 From: Ashish Sharma Date: Thu, 14 May 2026 17:16:28 +0800 Subject: [PATCH 1/2] fix(esp_http_server): prevent silent message drop in httpd_queue_work Closes https://github.com/espressif/esp-idf/issues/18563 --- components/esp_http_server/Kconfig | 15 +++- .../esp_http_server/src/esp_httpd_priv.h | 6 +- components/esp_http_server/src/httpd_main.c | 68 +++++++++++-------- .../test_apps/main/test_http_server.c | 66 ++++++++++++++++++ 4 files changed, 119 insertions(+), 36 deletions(-) diff --git a/components/esp_http_server/Kconfig b/components/esp_http_server/Kconfig index e5532104fdb..0b66ea3a95d 100644 --- a/components/esp_http_server/Kconfig +++ b/components/esp_http_server/Kconfig @@ -53,9 +53,18 @@ menu "HTTP Server" config HTTPD_QUEUE_WORK_BLOCKING bool "httpd_queue_work as blocking API" help - This makes httpd_queue_work() API to wait until a message space is available on UDP control socket. - It internally uses a counting semaphore with count set to `LWIP_UDP_RECVMBOX_SIZE` to achieve this. - This config will slightly change API behavior to block until message gets delivered on control socket. + Selects the wait policy for httpd_queue_work() when the UDP control-socket + mbox is at capacity. The HTTP server always uses a counting semaphore + (sized to LWIP_UDP_RECVMBOX_SIZE) to prevent silent mbox overflow drops + that would leak the caller's async-send callback context. + + When disabled (default): httpd_queue_work() returns ESP_FAIL immediately + if the mbox is full, so the caller can free its callback context. This + preserves the non-blocking semantics of httpd_ws_send_data_async(). + + When enabled: httpd_queue_work() blocks until a slot is available. Use + this only if your application can tolerate the call blocking and wants + guaranteed delivery instead of a fast-fail. config HTTPD_SERVER_EVENT_POST_TIMEOUT int "Time in millisecond to wait for posting event" diff --git a/components/esp_http_server/src/esp_httpd_priv.h b/components/esp_http_server/src/esp_httpd_priv.h index ca57ceea14a..f307170cfb5 100644 --- a/components/esp_http_server/src/esp_httpd_priv.h +++ b/components/esp_http_server/src/esp_httpd_priv.h @@ -17,6 +17,8 @@ #include #include "osal.h" +#include "freertos/semphr.h" +#include "sdkconfig.h" #ifdef __cplusplus extern "C" { @@ -129,9 +131,7 @@ struct httpd_data { httpd_config_t config; /*!< HTTPD server configuration */ int listen_fd; /*!< Server listener FD */ int ctrl_fd; /*!< Ctrl message receiver FD */ -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING - SemaphoreHandle_t ctrl_sock_semaphore; /*!< Ctrl socket semaphore */ -#endif + SemaphoreHandle_t ctrl_sock_semaphore; /*!< Ctrl mbox slot reservation (sized to LWIP_UDP_RECVMBOX_SIZE) */ int msg_fd; /*!< Ctrl message sender FD */ struct thread_data hd_td; /*!< Information for the HTTPD thread */ struct sock_db *hd_sd; /*!< The socket database */ diff --git a/components/esp_http_server/src/httpd_main.c b/components/esp_http_server/src/httpd_main.c index 61b44d086d9..ee2d91d2023 100644 --- a/components/esp_http_server/src/httpd_main.c +++ b/components/esp_http_server/src/httpd_main.c @@ -150,24 +150,27 @@ esp_err_t httpd_queue_work(httpd_handle_t handle, httpd_work_fn_t work, void *ar .hc_work = work, .hc_work_arg = arg, }; + + /* Reserve a slot in the control mbox before sending. In blocking mode + * the caller waits for a slot; in the default non-blocking mode we + * fail fast so the caller knows the work was not queued. */ #if CONFIG_HTTPD_QUEUE_WORK_BLOCKING - // Semaphore is acquired here and released after work function is executed. - if (xSemaphoreTake(hd->ctrl_sock_semaphore, portMAX_DELAY) == pdTRUE) { + const TickType_t wait = portMAX_DELAY; +#else + const TickType_t wait = 0; #endif - int ret = cs_send_to_ctrl_sock(hd->msg_fd, hd->config.ctrl_port, &msg, sizeof(msg)); - if (ret < 0) { - ESP_LOGW(TAG, LOG_FMT("failed to queue work")); -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING - xSemaphoreGive(hd->ctrl_sock_semaphore); -#endif - return ESP_FAIL; - } - return ESP_OK; -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING + if (xSemaphoreTake(hd->ctrl_sock_semaphore, wait) != pdTRUE) { + ESP_LOGW(TAG, LOG_FMT("ctrl socket queue full, work not queued")); + return ESP_FAIL; } - ESP_LOGE(TAG, "Unable to acquire semaphore"); - return ESP_FAIL; -#endif + + int ret = cs_send_to_ctrl_sock(hd->msg_fd, hd->config.ctrl_port, &msg, sizeof(msg)); + if (ret < 0) { + ESP_LOGW(TAG, LOG_FMT("failed to queue work")); + xSemaphoreGive(hd->ctrl_sock_semaphore); + return ESP_FAIL; + } + return ESP_OK; } esp_err_t httpd_get_client_list(httpd_handle_t handle, size_t *fds, int *client_fds) @@ -207,16 +210,16 @@ static void httpd_process_ctrl_msg(struct httpd_data *hd) int ret = recv(hd->ctrl_fd, &msg, sizeof(msg), 0); if (ret <= 0) { ESP_LOGW(TAG, LOG_FMT("error in recv (%d)"), errno); -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING + /* Returning the mbox slot to the producer side. xSemaphoreGive is a + * no-op once the counting semaphore is at its max, which keeps the + * accounting safe under producers that don't take (e.g. async + * handler wakeups, shutdown). */ xSemaphoreGive(hd->ctrl_sock_semaphore); -#endif return; } if (ret != sizeof(msg)) { ESP_LOGW(TAG, LOG_FMT("incomplete msg")); -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING xSemaphoreGive(hd->ctrl_sock_semaphore); -#endif return; } @@ -234,9 +237,7 @@ static void httpd_process_ctrl_msg(struct httpd_data *hd) default: break; } -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING xSemaphoreGive(hd->ctrl_sock_semaphore); -#endif } // Called for each session from httpd_server @@ -469,6 +470,10 @@ static void httpd_delete(struct httpd_data *hd) free(hd->err_handler_fns); free(ra->resp_hdrs); free(hd->hd_sd); + if (hd->ctrl_sock_semaphore) { + vSemaphoreDelete(hd->ctrl_sock_semaphore); + hd->ctrl_sock_semaphore = NULL; + } /* Free registered URI handlers */ httpd_unregister_all_uri_handlers(hd); @@ -504,18 +509,18 @@ esp_err_t httpd_start(httpd_handle_t *handle, const httpd_config_t *config) /* Failed to allocate memory */ return ESP_ERR_HTTPD_ALLOC_MEM; } -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING - /* Using a Counting Semaphore with count equals CONFIG_LWIP_UDP_RECVMBOX_SIZE - * as the number of UDP messages which can be stored is equal to UDP mailbox size. - * Using this, we can make sure that the work function is always received by the ctrl socket. - */ + /* Counting semaphore sized to the UDP control-socket recv mbox. Each + * httpd_queue_work() take reserves one mbox slot; httpd_process_ctrl_msg() + * gives one back per drain. This bounds the producer to the mbox capacity + * and prevents silent lwIP-mbox overflow drops that would otherwise leak + * the caller's async-send context. Always created so the default + * (non-blocking) httpd_queue_work() path can also rely on it. */ hd->ctrl_sock_semaphore = xSemaphoreCreateCounting(CONFIG_LWIP_UDP_RECVMBOX_SIZE, CONFIG_LWIP_UDP_RECVMBOX_SIZE); if (hd->ctrl_sock_semaphore == NULL) { ESP_LOGE(TAG, "Failed to create Semaphore"); httpd_delete(hd); return ESP_ERR_HTTPD_ALLOC_MEM; } -#endif if (httpd_server_init(hd) != ESP_OK) { httpd_delete(hd); @@ -529,6 +534,12 @@ esp_err_t httpd_start(httpd_handle_t *handle, const httpd_config_t *config) httpd_thread, hd, hd->config.core_id, hd->config.task_caps) != ESP_OK) { + /* Close the open socket */ + close(hd->listen_fd); + /* Close the control socket */ + cs_free_ctrl_sock(hd->ctrl_fd); + /* Close the message socket */ + close(hd->msg_fd); /* Failed to launch task */ httpd_delete(hd); return ESP_ERR_HTTPD_TASK; @@ -582,9 +593,6 @@ esp_err_t httpd_stop(httpd_handle_t handle) } ESP_LOGD(TAG, LOG_FMT("server stopped")); -#if CONFIG_HTTPD_QUEUE_WORK_BLOCKING - vSemaphoreDelete(hd->ctrl_sock_semaphore); -#endif httpd_delete(hd); esp_http_server_dispatch_event(HTTP_SERVER_EVENT_STOP, NULL, 0); return ESP_OK; diff --git a/components/esp_http_server/test_apps/main/test_http_server.c b/components/esp_http_server/test_apps/main/test_http_server.c index 0b4da34bc95..067c341e754 100644 --- a/components/esp_http_server/test_apps/main/test_http_server.c +++ b/components/esp_http_server/test_apps/main/test_http_server.c @@ -9,6 +9,10 @@ #include #include #include +#include +#include "esp_log.h" +#include "freertos/FreeRTOS.h" +#include "freertos/semphr.h" #ifdef CONFIG_HTTPD_WS_SUPPORT #include "../../src/esp_httpd_priv.h" @@ -348,6 +352,68 @@ TEST_CASE("httpd_resp_set_type rejects CRLF in content type", "[HTTP SERVER][sec httpd_resp_set_type(&fake_req, "text/html\nX-Injected: pwned")); } +/* ---- httpd_queue_work backpressure ---- */ + +static SemaphoreHandle_t s_qw_gate; +static volatile int s_qw_work_runs; + +static void qw_blocking_work(void *arg) +{ + /* Hold the httpd thread inside this work fn so the ctrl-socket mbox + * stops draining. Auto-release after 2 s as a safety net in case the + * test asserts mid-way and never reaches the explicit give. */ + xSemaphoreTake((SemaphoreHandle_t)arg, pdMS_TO_TICKS(2000)); +} + +static void qw_counting_work(void *arg) +{ + (void)arg; + s_qw_work_runs++; +} + +TEST_CASE("httpd_queue_work fast-fails on ctrl mbox saturation", "[HTTP SERVER]") +{ + test_case_uses_tcpip(); + + httpd_handle_t hd = NULL; + httpd_config_t config = HTTPD_DEFAULT_CONFIG(); + TEST_ASSERT_EQUAL(ESP_OK, httpd_start(&hd, &config)); + + s_qw_gate = xSemaphoreCreateBinary(); + TEST_ASSERT_NOT_NULL(s_qw_gate); + s_qw_work_runs = 0; + + /* Park the httpd thread in a blocked work item so the mbox can fill. */ + TEST_ASSERT_EQUAL(ESP_OK, httpd_queue_work(hd, qw_blocking_work, s_qw_gate)); + vTaskDelay(pdMS_TO_TICKS(100)); + + /* Spam queue_work past the mbox cap; first ones succeed, rest must + * return ESP_FAIL synchronously (default non-blocking behavior). */ + int ok_count = 0; + int fail_count = 0; + for (int i = 0; i < CONFIG_LWIP_UDP_RECVMBOX_SIZE * 2 + 4; i++) { + esp_err_t err = httpd_queue_work(hd, qw_counting_work, NULL); + if (err == ESP_OK) { + ok_count++; + } else { + TEST_ASSERT_EQUAL(ESP_FAIL, err); + fail_count++; + } + } + TEST_ASSERT_GREATER_THAN(0, ok_count); + TEST_ASSERT_LESS_OR_EQUAL(CONFIG_LWIP_UDP_RECVMBOX_SIZE, ok_count); + TEST_ASSERT_GREATER_THAN(0, fail_count); + + /* Release the parked work; every accepted item must now actually run. */ + xSemaphoreGive(s_qw_gate); + vTaskDelay(pdMS_TO_TICKS(300)); + TEST_ASSERT_EQUAL(ok_count, s_qw_work_runs); + + vSemaphoreDelete(s_qw_gate); + s_qw_gate = NULL; + TEST_ASSERT_EQUAL(ESP_OK, httpd_stop(hd)); +} + #ifdef CONFIG_HTTPD_WS_SUPPORT TEST_CASE("WS recv failure marks close without dispatching handler", "[HTTP SERVER][websocket]") { From dedb02dd254a39b99f229897b2f2bcaca8f0c1ce Mon Sep 17 00:00:00 2001 From: Ashish Sharma Date: Tue, 26 May 2026 17:17:03 +0800 Subject: [PATCH 2/2] fix(esp_http_server): take ctrl_sock_semaphore on shutdown and async wake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit httpd_stop() and httpd_req_async_handler_complete() both pushed messages onto the control mbox via cs_send_to_ctrl_sock() without reserving a slot in ctrl_sock_semaphore. Once the silent-drop fix made the semaphore unconditional, the bypass became a real bug: when the mbox is saturated by pending httpd_queue_work() items the unguarded sendto() can return ENOBUFS, and even when it succeeds it leaves the semaphore overstating free slots until the consumer drains the message — a window during which a concurrent httpd_queue_work() can take a slot but still find the mbox full. Acquire the semaphore (portMAX_DELAY) before both sends and give it back on send failure so the take/give invariant is preserved. The httpd task is the consumer in both paths, so blocking is bounded and deadlock-free. Reword the stale "no-op give on full" comment in httpd_process_ctrl_msg() to reflect that only the recv-error path relies on the cap behavior now. --- components/esp_http_server/src/httpd_main.c | 22 +++++++++++++++------ components/esp_http_server/src/httpd_txrx.c | 10 ++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/components/esp_http_server/src/httpd_main.c b/components/esp_http_server/src/httpd_main.c index ee2d91d2023..890e81cdf9d 100644 --- a/components/esp_http_server/src/httpd_main.c +++ b/components/esp_http_server/src/httpd_main.c @@ -210,10 +210,10 @@ static void httpd_process_ctrl_msg(struct httpd_data *hd) int ret = recv(hd->ctrl_fd, &msg, sizeof(msg), 0); if (ret <= 0) { ESP_LOGW(TAG, LOG_FMT("error in recv (%d)"), errno); - /* Returning the mbox slot to the producer side. xSemaphoreGive is a - * no-op once the counting semaphore is at its max, which keeps the - * accounting safe under producers that don't take (e.g. async - * handler wakeups, shutdown). */ + /* No packet was actually consumed from the mbox here, so this give + * is unbalanced. It's tolerated because the counting semaphore is + * capped at its max — excess gives become no-ops. Spurious recv + * errors after select() are rare in practice. */ xSemaphoreGive(hd->ctrl_sock_semaphore); return; } @@ -561,9 +561,19 @@ esp_err_t httpd_stop(httpd_handle_t handle) struct httpd_ctrl_data msg; memset(&msg, 0, sizeof(msg)); msg.hc_msg = HTTPD_CTRL_SHUTDOWN; - int ret = 0; - if ((ret = cs_send_to_ctrl_sock(hd->msg_fd, hd->config.ctrl_port, &msg, sizeof(msg))) < 0) { + + /* Reserve a slot in the ctrl mbox before sending so we never push past + * its capacity. Blocking is safe: the httpd task is the consumer and + * keeps draining the mbox until it observes HTTPD_CTRL_SHUTDOWN. */ + if (xSemaphoreTake(hd->ctrl_sock_semaphore, portMAX_DELAY) != pdTRUE) { + ESP_LOGE(TAG, "Failed to acquire ctrl socket semaphore"); + return ESP_FAIL; + } + + int ret = cs_send_to_ctrl_sock(hd->msg_fd, hd->config.ctrl_port, &msg, sizeof(msg)); + if (ret < 0) { ESP_LOGE(TAG, "Failed to send shutdown signal err=%d", ret); + xSemaphoreGive(hd->ctrl_sock_semaphore); return ESP_FAIL; } diff --git a/components/esp_http_server/src/httpd_txrx.c b/components/esp_http_server/src/httpd_txrx.c index b0998d420e2..d53ffc16836 100644 --- a/components/esp_http_server/src/httpd_txrx.c +++ b/components/esp_http_server/src/httpd_txrx.c @@ -730,9 +730,19 @@ esp_err_t httpd_req_async_handler_complete(httpd_req_t *r) // will now re-add this FD to its select() descriptor list. This ensures that subsequent requests // on the same FD are processed correctly struct httpd_ctrl_data msg = {.hc_msg = HTTPD_CTRL_MAX}; + + /* Reserve an mbox slot so we don't overrun ctrl_sock_semaphore's + * accounting and starve concurrent httpd_queue_work() producers. The + * httpd main task is the consumer and will drain the mbox shortly. */ + if (xSemaphoreTake(hd->ctrl_sock_semaphore, portMAX_DELAY) != pdTRUE) { + ESP_LOGW(TAG, LOG_FMT("failed to acquire ctrl socket semaphore")); + return ESP_FAIL; + } + int ret = cs_send_to_ctrl_sock(msg_fd, port, &msg, sizeof(msg)); if (ret < 0) { ESP_LOGW(TAG, LOG_FMT("failed to send socket notification")); + xSemaphoreGive(hd->ctrl_sock_semaphore); return ESP_FAIL; }