From 863ab2f7c94236e7b1399806781cb7498c912fb9 Mon Sep 17 00:00:00 2001 From: Will Miles Date: Tue, 21 Jul 2026 20:39:35 -0400 Subject: [PATCH 1/2] fix(http): prevent use-after-free from synchronous abort/close() (AsyncTCP 3.5.0) Ensure that AsyncWebRequest objects are held in scope until the end of their handler functions, avoiding any possible use-after-free cases when aborted or closed. This is managed by using `std::shared_ptr` to allow a scoped lifecycle "lock" to be taken without an OS mutex. --- src/ESPAsyncWebServer.h | 14 ++++++++++++-- src/WebRequest.cpp | 20 +++++++++++--------- src/WebServer.cpp | 7 ++----- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/ESPAsyncWebServer.h b/src/ESPAsyncWebServer.h index b19989d7..4c418943 100644 --- a/src/ESPAsyncWebServer.h +++ b/src/ESPAsyncWebServer.h @@ -508,11 +508,22 @@ class AsyncWebServerRequest { static bool _getEtag(File gzFile, char *eTag); + // Constructor is private to ensure factory is used to create shared_ptrs + AsyncWebServerRequest(AsyncWebServer *, AsyncClient *); + public: File _tempFile; void *_tempObject; - AsyncWebServerRequest(AsyncWebServer *, AsyncClient *); + // Factory function + static std::shared_ptr create(AsyncWebServer *server, AsyncClient *client) { + AsyncWebServerRequest *req = new (std::nothrow) AsyncWebServerRequest(server, client); + if (req) { + req->_this = std::shared_ptr(req); // store shared pointer to this request + return req->_this; + } + return {}; // empty shared_ptr + } ~AsyncWebServerRequest(); AsyncClient *client() { @@ -1763,7 +1774,6 @@ class AsyncWebServer : public AsyncMiddlewareChain { void reset(); // remove all writers and handlers, with onNotFound/onFileUpload/onRequestBody - void _handleDisconnect(AsyncWebServerRequest *request); void _attachHandler(AsyncWebServerRequest *request); void _rewriteRequest(AsyncWebServerRequest *request); }; diff --git a/src/WebRequest.cpp b/src/WebRequest.cpp index 2cba7d32..39010d25 100644 --- a/src/WebRequest.cpp +++ b/src/WebRequest.cpp @@ -17,8 +17,6 @@ static inline bool isParamChar(char c) { return ((c) && ((c) != '{') && ((c) != '[') && ((c) != '&') && ((c) != '=')); } -static void doNotDelete(AsyncWebServerRequest *) {} - using namespace asyncsrv; enum { @@ -108,8 +106,6 @@ AsyncWebServerRequest::~AsyncWebServerRequest() { _response = nullptr; } - _this.reset(); - if (_tempObject != NULL) { free(_tempObject); } @@ -124,6 +120,8 @@ AsyncWebServerRequest::~AsyncWebServerRequest() { } void AsyncWebServerRequest::_onData(void *buf, size_t len) { + std::shared_ptr self = _this; // Ensure we stay in scope over the function + // 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). @@ -240,6 +238,7 @@ void AsyncWebServerRequest::_onData(void *buf, size_t len) { } void AsyncWebServerRequest::_onPoll() { + std::shared_ptr self = _this; // Ensure we stay in scope over the function // os_printf("p\n"); if (_response && _client && _client->canSend()) { _response->_ack(this, 0, 0); @@ -252,6 +251,8 @@ void AsyncWebServerRequest::_onAck(size_t len, uint32_t time) { return; } + std::shared_ptr self = _this; // Ensure we stay in scope over the function + if (!_response->_finished()) { _response->_ack(this, len, time); // recheck if response has just completed, close connection @@ -271,6 +272,7 @@ void AsyncWebServerRequest::_onError(int8_t error) { void AsyncWebServerRequest::_onTimeout(uint32_t time) { (void)time; // os_printf("TIMEOUT: %u, state: %s\n", time, _client->stateToString()); + // We do not need to lock the shared pointer here as we do no work after closing the client _client->close(); } @@ -283,7 +285,7 @@ void AsyncWebServerRequest::_onDisconnect() { if (_onDisconnectfn) { _onDisconnectfn(); } - _server->_handleDisconnect(this); + this->_this.reset(); // release shared pointer to this request, allowing for object destruction } void AsyncWebServerRequest::_addGetParams(const String ¶ms) { @@ -1054,9 +1056,6 @@ AsyncWebServerRequestPtr AsyncWebServerRequest::pause() { return _this; } client()->setRxTimeout(0); - // this shared ptr will hold the request pointer until it gets destroyed following a disconnect. - // this is just used as a holder providing weak observers, so the deleter is a no-op. - _this = std::shared_ptr(this, doNotDelete); _paused = true; return _this; } @@ -1065,7 +1064,6 @@ void AsyncWebServerRequest::abort() { if (!_sent) { _sent = true; _paused = false; - _this.reset(); async_ws_log_v("Abort request: %s", _url.c_str()); _client->abort(); } @@ -1327,7 +1325,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 +1351,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; diff --git a/src/WebServer.cpp b/src/WebServer.cpp index cbdca129..5c2db8c7 100644 --- a/src/WebServer.cpp +++ b/src/WebServer.cpp @@ -4,6 +4,7 @@ #include "ESPAsyncWebServer.h" #include "WebHandlerImpl.h" +#include #include #include @@ -47,7 +48,7 @@ AsyncWebServer::AsyncWebServer(uint16_t port) : _server(port) { return; } c->setRxTimeout(3); - AsyncWebServerRequest *r = new AsyncWebServerRequest((AsyncWebServer *)s, c); + std::shared_ptr r = AsyncWebServerRequest::create(static_cast(s), c); if (r == NULL) { c->abort(); delete c; @@ -127,10 +128,6 @@ void AsyncWebServer::beginSecure(const char *cert, const char *key, const char * } #endif -void AsyncWebServer::_handleDisconnect(AsyncWebServerRequest *request) { - delete request; -} - void AsyncWebServer::_rewriteRequest(AsyncWebServerRequest *request) { // the last rewrite that matches the request will be used // we do not break the loop to allow for multiple rewrites to be applied and only the last one to be used (allows overriding) From 1f75bc61c01c7786e5a7777d064201a66dcb2e1f Mon Sep 17 00:00:00 2001 From: Mathieu Carbou Date: Wed, 22 Jul 2026 12:33:28 +0200 Subject: [PATCH 2/2] fix(http): fix double-free, _onTimeout and _onDisconnect safety in shared_ptr lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes to the shared_ptr-based request lifecycle introduced in the previous commit: 1. Double-free in WebSocket / EventSource handoff (CRITICAL) create() sets _this as an owning shared_ptr, but AsyncWebSocket::_newClient() and AsyncEventSourceClient's constructor still called `delete request` directly. When the shared_ptr later dropped its last reference it deleted the already-freed request a second time → heap corruption: CORRUPT HEAP assertion in std::list destroyed a second time → multi_heap_free() Fix: added a public release() method that calls _this.reset(). Replaced both `delete request` calls with `request->release()` so destruction goes through the shared_ptr exactly once. If a local shared_ptr hold (e.g. in _onData/_onPoll/_onAck) is on the stack, the request stays alive until that hold goes out of scope. 2. _onTimeout missing shared_ptr hold _onTimeout() called _client->close() without holding a local shared_ptr. close() triggers a synchronous _onDisconnect() → _this.reset() which can drop the last reference and destruct the request (and delete the AsyncClient) while close() is still on the stack. Added the same `std::shared_ptr self = _this;` hold used by _onData/_onPoll/_onAck. 3. _onAck shared_ptr hold placed after early return The `self = _this` hold was after the `if (!_response) return;` check, unlike _onData and _onPoll where it is at the top. Moved it to the top of the function for consistency and to guard the early-return path against future regressions. 4. _onDisconnect() self-destruction during member execution _onDisconnect() did `this->_this.reset()` as its last statement. If _this was the last owning shared_ptr, reset() destructed *this while the member function was still executing (undefined behavior). Fix: take a local copy `self = _this` first, run the user callback with _this still valid, then reset the member. The local copy keeps the object alive until the function returns, at which point the shared_ptr destructor runs safely. --- src/AsyncEventSource.cpp | 7 +++++-- src/AsyncWebSocket.cpp | 5 ++++- src/ESPAsyncWebServer.h | 10 ++++++++++ src/WebRequest.cpp | 13 +++++++++---- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/AsyncEventSource.cpp b/src/AsyncEventSource.cpp index dc15ef76..1144b79a 100644 --- a/src/AsyncEventSource.cpp +++ b/src/AsyncEventSource.cpp @@ -186,8 +186,11 @@ AsyncEventSourceClient::AsyncEventSourceClient(AsyncWebServerRequest *request, A _server->_addClient(this); _client->setNoDelay(true); - // delete AsyncWebServerRequest object (and bound response) since we have the ownership on client connection now - delete request; + // Release the AsyncWebServerRequest (and bound response) since we have the ownership on client connection now. + // Use release() instead of `delete request` because _this is now an owning + // shared_ptr (set in create()). A manual delete would cause a double-free + // when the shared_ptr later drops its last reference. + request->release(); } AsyncEventSourceClient::~AsyncEventSourceClient() { diff --git a/src/AsyncWebSocket.cpp b/src/AsyncWebSocket.cpp index 9268eb19..a47880cd 100644 --- a/src/AsyncWebSocket.cpp +++ b/src/AsyncWebSocket.cpp @@ -1001,7 +1001,10 @@ AsyncWebSocketClient *AsyncWebSocket::_newClient(AsyncWebServerRequest *request) // we've just detached AsyncTCP client from AsyncWebServerRequest _handleEvent(&_clients.back(), WS_EVT_CONNECT, request, NULL, 0); // after user code completed CONNECT event callback we can delete req/response objects - delete request; + // Use release() instead of `delete request` because _this is now an owning + // shared_ptr (set in create()). A manual delete would cause a double-free + // when the shared_ptr later drops its last reference. + request->release(); return &_clients.back(); } diff --git a/src/ESPAsyncWebServer.h b/src/ESPAsyncWebServer.h index 4c418943..7ac465d0 100644 --- a/src/ESPAsyncWebServer.h +++ b/src/ESPAsyncWebServer.h @@ -526,6 +526,16 @@ class AsyncWebServerRequest { } ~AsyncWebServerRequest(); + // Release the self-referential shared_ptr that owns this request. + // Used by WebSocket / EventSource handoff paths (which used to call `delete request` + // directly) to trigger destruction via the shared_ptr instead of a manual delete, + // avoiding a double-free now that _this is an owning shared_ptr. + // If a local shared_ptr hold (e.g. in _onData/_onPoll/_onAck) is on the stack, + // the request stays alive until that hold goes out of scope. + void release() { + _this.reset(); + } + AsyncClient *client() { return _client; } diff --git a/src/WebRequest.cpp b/src/WebRequest.cpp index 39010d25..110680db 100644 --- a/src/WebRequest.cpp +++ b/src/WebRequest.cpp @@ -246,13 +246,12 @@ void AsyncWebServerRequest::_onPoll() { } void AsyncWebServerRequest::_onAck(size_t len, uint32_t time) { + std::shared_ptr self = _this; // Ensure we stay in scope over the function // os_printf("a:%u:%u\n", len, time); if (!_response) { return; } - std::shared_ptr self = _this; // Ensure we stay in scope over the function - if (!_response->_finished()) { _response->_ack(this, len, time); // recheck if response has just completed, close connection @@ -270,9 +269,9 @@ void AsyncWebServerRequest::_onError(int8_t error) { } void AsyncWebServerRequest::_onTimeout(uint32_t time) { + std::shared_ptr self = _this; // Ensure we stay in scope over the function (void)time; // os_printf("TIMEOUT: %u, state: %s\n", time, _client->stateToString()); - // We do not need to lock the shared pointer here as we do no work after closing the client _client->close(); } @@ -282,10 +281,16 @@ void AsyncWebServerRequest::onDisconnect(ArDisconnectHandler fn) { void AsyncWebServerRequest::_onDisconnect() { async_ws_log_v("onDisconnect() cb for request: %s", _url.c_str()); + // Take a local copy of _this before resetting the member: if _this is the + // last owning shared_ptr, resetting it would destruct *this while this + // member function is still executing. The local copy keeps us alive until + // the function returns, then the shared_ptr destructor runs safely. + std::shared_ptr self = _this; if (_onDisconnectfn) { _onDisconnectfn(); } - this->_this.reset(); // release shared pointer to this request, allowing for object destruction + _this.reset(); // release shared pointer to this request, allowing for object destruction + // 'self' goes out of scope here — if it was the last owner, the request is deleted } void AsyncWebServerRequest::_addGetParams(const String ¶ms) {