From 366b032392c4ebbf7434c49d321e58835656332f Mon Sep 17 00:00:00 2001 From: Mathieu Carbou Date: Tue, 21 Jul 2026 17:32:51 +0200 Subject: [PATCH] fix(http): prevent use-after-free from synchronous abort()/close() (AsyncTCP 3.5.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AsyncTCP 3.5.0 (commit ESP32Async/AsyncTCP@beb0e95) changed abort() to fire the error and disconnect (discard) callbacks synchronously instead of deferring them to the async event queue: int8_t AsyncClient::abort() { int8_t err = _tcp_abort(&_pcb, this); if (err != ERR_CONN) { _error(ERR_ABRT); // <-- now synchronous: calls onError + onDisconnect } return err; } close() has the same behaviour — it calls _discard_cb inline when the pcb is closed successfully. This breaks ESPAsyncWebServer's HTTP request path. When abort() (or close() from a response) is called from inside a TCP callback (e.g. _onData -> _parseLine -> abort()), the synchronous _onDisconnect() runs _server->_handleDisconnect(this) -> delete this -> ~AsyncWebServerRequest() -> delete _client -> ~AsyncClient(), destroying the AsyncClient while it is still on the call stack inside _recv()/_sent()/_poll(). After the callback returns, AsyncTCP accesses the freed client (use-after-free), corrupting the heap. Over time this can cause LwIP's tcp_accept() to receive a NULL pcb: [E][AsyncTCP.cpp:1572] tcp_accept(): _accept failed: pcb is NULL All ~15 abort() call sites in WebRequest.cpp (SSL detection, null bytes in headers, allocation failures, multipart/chunked parse errors, boundary validation, auth header allocation), plus close() calls in _onAck/_onTimeout and response write paths, are re-entrancy hazards. Fix: defer `delete this` when _onDisconnect() runs re-entrantly. - Added two flags to AsyncWebServerRequest: _inCallback — set by each top-level TCP callback _disconnectPending — set by _onDisconnect() when it cannot delete yet - _onDisconnect(): if _inCallback is true, sets _disconnectPending and returns without deleting; the AsyncClient stays alive until the stack unwinds. If _inCallback is false (normal async disconnect), it deletes immediately as before. - New helper _onTcpCallbackExit() consolidates the deferred-delete check at every exit point of _onData, _onAck, _onPoll, _onTimeout: it clears _inCallback, performs `delete this` via _handleDisconnect() if pending, and returns true so the caller can `return` without touching `this`. - Parse functions (_onData body loop, _parseLine, _parseChunkedBytes, _parseMultipartPostByte) check _disconnectPending and break/return before accessing members after a nested abort(). - Fixed requestAuthentication(): two paths did `abort(); break; send(r);` which accessed `this` after deletion and leaked the response. Now `delete r; abort(); return;`. The WebSocket side has the same class of issue and is addressed separately in PR #462. Builds: latest-asynctcp (AsyncTCP 3.5.0), arduino-3, esp8266. --- src/ESPAsyncWebServer.h | 17 +++++++++ src/WebRequest.cpp | 82 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/src/ESPAsyncWebServer.h b/src/ESPAsyncWebServer.h index b19989d7..6e93aaa0 100644 --- a/src/ESPAsyncWebServer.h +++ b/src/ESPAsyncWebServer.h @@ -438,6 +438,15 @@ class AsyncWebServerRequest { bool _paused = false; // request is paused (request continuation) std::shared_ptr _this; // shared pointer to this request + // Re-entrancy guard for AsyncTCP 3.5.0+ where abort()/close() fire the + // disconnect (discard) callback synchronously. If _onDisconnect() runs + // while we are executing inside the async_tcp task (_inAsyncTcpTask == + // true), it must NOT `delete this` — the AsyncClient is still on the call + // stack. Instead it sets _disconnectPending and the top-level callback + // performs the delete after unwinding. + bool _inAsyncTcpTask = false; + bool _disconnectPending = false; + String _temp; uint8_t _parseState; @@ -485,6 +494,14 @@ class AsyncWebServerRequest { uint8_t _chunkedLastChar; bool _parseChunkedBytes(uint8_t *data, size_t len); + // Called at every exit point of a top-level async_tcp task callback + // (_onData, _onAck, _onPoll, _onTimeout). Returns true if the request is + // being disconnected (abort()/close() ran _onDisconnect() re-entrantly + // during the callback), in which case the caller must `return` immediately + // without touching `this` — this method has already performed `delete this` + // via _handleDisconnect(). + bool _onAsyncTcpTaskExit(); + void _onPoll(); void _onAck(size_t len, uint32_t time); void _onError(int8_t error); diff --git a/src/WebRequest.cpp b/src/WebRequest.cpp index 2cba7d32..1afb4ef0 100644 --- a/src/WebRequest.cpp +++ b/src/WebRequest.cpp @@ -124,12 +124,17 @@ AsyncWebServerRequest::~AsyncWebServerRequest() { } void AsyncWebServerRequest::_onData(void *buf, size_t len) { + _inAsyncTcpTask = true; + // SSL/TLS handshake detection #ifndef ASYNC_TCP_SSL_ENABLED if (_parseState == PARSE_REQ_START && len && ((uint8_t *)buf)[0] == 0x16) { // 0x16 indicates a Handshake message (SSL/TLS). async_ws_log_d("SSL/TLS handshake detected: resetting connection"); _parseState = PARSE_REQ_FAIL; abort(); + if (_onAsyncTcpTaskExit()) { + return; + } return; } #endif @@ -145,6 +150,9 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { if (!str[i]) { _parseState = PARSE_REQ_FAIL; abort(); + if (_onAsyncTcpTaskExit()) { + return; + } return; } if (str[i] == '\n') { @@ -158,6 +166,9 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { async_ws_log_e("Failed to allocate"); _parseState = PARSE_REQ_FAIL; abort(); + if (_onAsyncTcpTaskExit()) { + return; + } return; } _temp.concat(str); @@ -167,6 +178,11 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { _temp.concat(str); _temp.trim(); _parseLine(); + // _parseLine() may call abort() on a parse error; bail out before + // touching any member if the request is being disconnected. + if (_disconnectPending) { + break; + } if (++i < len) { // Still have more buffer to process buf = str + i; @@ -177,9 +193,13 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { } else if (_parseState == PARSE_REQ_BODY) { if (_chunkedParseState != CHUNK_NONE) { if (_parseChunkedBytes((uint8_t *)buf, len)) { - _parseState = PARSE_REQ_END; - _runMiddlewareChain(); - _send(); + // _parseChunkedBytes may have called abort() on a parse error; + // if so, _send()/_runMiddlewareChain() must be skipped. + if (!_disconnectPending) { + _parseState = PARSE_REQ_END; + _runMiddlewareChain(); + _send(); + } } break; } @@ -193,6 +213,10 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { size_t i; for (i = 0; i < len; i++) { _parseMultipartPostByte(((uint8_t *)buf)[i], i == len - 1); + // _parseMultipartPostByte may call abort() on a parse error. + if (_disconnectPending) { + break; + } _parsedLength++; } } else { @@ -229,6 +253,9 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { _parsedLength += len; } } + if (_disconnectPending) { + break; + } if (_parsedLength == _contentLength) { _parseState = PARSE_REQ_END; _runMiddlewareChain(); @@ -237,18 +264,30 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { } break; } + + if (_onAsyncTcpTaskExit()) { + return; + } } void AsyncWebServerRequest::_onPoll() { + _inAsyncTcpTask = true; // os_printf("p\n"); if (_response && _client && _client->canSend()) { _response->_ack(this, 0, 0); } + if (_onAsyncTcpTaskExit()) { + return; + } } void AsyncWebServerRequest::_onAck(size_t len, uint32_t time) { + _inAsyncTcpTask = true; // os_printf("a:%u:%u\n", len, time); if (!_response) { + if (_onAsyncTcpTaskExit()) { + return; + } return; } @@ -262,6 +301,9 @@ void AsyncWebServerRequest::_onAck(size_t len, uint32_t time) { // this will close responses that were complete via a single _send() call _client->close(); // this will trigger _onDisconnect() and object destruction } + if (_onAsyncTcpTaskExit()) { + return; + } } void AsyncWebServerRequest::_onError(int8_t error) { @@ -269,21 +311,49 @@ void AsyncWebServerRequest::_onError(int8_t error) { } void AsyncWebServerRequest::_onTimeout(uint32_t time) { + _inAsyncTcpTask = true; (void)time; // os_printf("TIMEOUT: %u, state: %s\n", time, _client->stateToString()); _client->close(); + if (_onAsyncTcpTaskExit()) { + return; + } } void AsyncWebServerRequest::onDisconnect(ArDisconnectHandler fn) { _onDisconnectfn = fn; } +bool AsyncWebServerRequest::_onAsyncTcpTaskExit() { + _inAsyncTcpTask = false; + // AsyncTCP 3.5.0+ fires the discard (onDisconnect) callback synchronously + // from abort()/close(). If that happened during the callback we just ran, + // _onDisconnect() set _disconnectPending instead of deleting `this` (because + // the AsyncClient was still on the call stack). Now that we've unwound, it + // is safe to perform the deletion. Returns true if `this` was deleted — the + // caller must not touch any member afterward. + if (_disconnectPending) { + _server->_handleDisconnect(this); // delete this + return true; + } + return false; +} + void AsyncWebServerRequest::_onDisconnect() { async_ws_log_v("onDisconnect() cb for request: %s", _url.c_str()); if (_onDisconnectfn) { _onDisconnectfn(); } - _server->_handleDisconnect(this); + // AsyncTCP 3.5.0+ fires this callback synchronously from abort()/close(). + // If we are executing inside the async_tcp task (_inAsyncTcpTask), the + // AsyncClient is still on the call stack and `delete this` here would + // destroy it mid-call (use-after-free). Defer the deletion to + // _onAsyncTcpTaskExit(). + if (_inAsyncTcpTask) { + _disconnectPending = true; + } else { + _server->_handleDisconnect(this); + } } void AsyncWebServerRequest::_addGetParams(const String ¶ms) { @@ -1327,7 +1397,9 @@ void AsyncWebServerRequest::requestAuthentication(AsyncAuthType method, const ch r->addHeader(T_WWW_AUTH, header.c_str()); } else { async_ws_log_e("Failed to allocate"); + delete r; abort(); + return; } break; @@ -1351,7 +1423,9 @@ void AsyncWebServerRequest::requestAuthentication(AsyncAuthType method, const ch r->addHeader(T_WWW_AUTH, header.c_str()); } else { async_ws_log_e("Failed to allocate"); + delete r; abort(); + return; } } break;