fix(http): prevent use-after-free from synchronous abort/close()

src/AsyncEventSource.cpp

@@ -147,11 +147,8 @@ size_t AsyncEventSourceMessage::send(AsyncClient *client) {
 
 // Client
 
-AsyncEventSourceClient::AsyncEventSourceClient(AsyncWebServerRequest *request, AsyncEventSource *server) : _client(request->clientRelease()), _server(server) {
-
-  if (request->hasHeader(T_Last_Event_ID)) {
-    _lastId = atoi(request->getHeader(T_Last_Event_ID)->value().c_str());
-  }
+AsyncEventSourceClient::AsyncEventSourceClient(AsyncClient *client, AsyncEventSource *server, uint32_t lastId)
+  : _client(client), _server(server), _lastId(lastId) {
 
   _client->setRxTimeout(0);
   _client->onError(NULL, NULL);
@@ -186,8 +183,6 @@ 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;
 }
 
 AsyncEventSourceClient::~AsyncEventSourceClient() {
@@ -478,24 +473,12 @@ AsyncEventSourceResponse::AsyncEventSourceResponse(AsyncEventSource *server) : _
 void AsyncEventSourceResponse::_respond(AsyncWebServerRequest *request) {
   String out;
   _assembleHead(out, request->version());
-  // unbind client's onAck callback from AsyncWebServerRequest's, we will destroy it on next callback and steal the client,
-  // can't do it now 'cause now we are in AsyncWebServerRequest::_onAck 's stack actually
-  // here we are loosing time on one RTT delay, but with current design we can't get rid of Req/Resp objects other way
-  _request = request;
-  request->client()->onAck(
-    [](void *r, AsyncClient *c, size_t len, uint32_t time) {
-      if (len) {
-        static_cast<AsyncEventSourceResponse *>(r)->_switchClient();
-      }
-    },
-    this
-  );
+  uint32_t lastId = 0;
+  if (request->hasHeader(T_Last_Event_ID)) {
+    lastId = strtoul(request->getHeader(T_Last_Event_ID)->value().c_str(), nullptr, 10);
+  }
   request->client()->write(out.c_str(), _headLength);
-  _state = RESPONSE_WAIT_ACK;
+  // Add a new AsyncEventSourceClient to the server's list of clients
+  // This adopts the ownership of the AsyncTCP's client pointer from `request` parameter
+  new AsyncEventSourceClient(request->clientRelease(), _server, lastId);
 }
-
-void AsyncEventSourceResponse::_switchClient() {
-  // AsyncEventSourceClient c-tor will take the ownership of AsyncTCP's client connection
-  new AsyncEventSourceClient(_request, _server);
-  // AsyncEventSourceClient c-tor would also delete _request and *this
-};

src/AsyncEventSource.h

@@ -144,12 +144,13 @@ class AsyncEventSourceClient {
 public:
   /**
    * @brief Construct a new Async Event Source Client object
-   * @note constructor would take the ownership of of AsyncTCP's client pointer from `request` parameter and call delete on it!
+   * @note constructor is normally passed a client object from AsyncWebServerRequest::releaseClient(); see AsyncEventSourceResponse::_respond()
    *
-   * @param request
+   * @param client
    * @param server
+   * @param lastId
    */
-  AsyncEventSourceClient(AsyncWebServerRequest *request, AsyncEventSource *server);
+  AsyncEventSourceClient(AsyncClient *client, AsyncEventSource *server, uint32_t lastId = 0);
   ~AsyncEventSourceClient();
 
   /**
@@ -319,9 +320,6 @@ class AsyncEventSource : public AsyncWebHandler {
 class AsyncEventSourceResponse : public AsyncWebServerResponse {
 private:
   AsyncEventSource *_server;
-  AsyncWebServerRequest *_request;
-  // this call back will switch AsyncTCP client to SSE
-  void _switchClient();
 
 public:
   AsyncEventSourceResponse(AsyncEventSource *server);

src/AsyncWebSocket.cpp

@@ -535,6 +535,7 @@ void AsyncWebSocketClient::close(uint16_t code, const char *message) {
       if (c) {
         c->abort();
       }
+      return;
     }
   }
   _queueControl(WS_DISCONNECT);
@@ -680,7 +681,9 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
           "[%s][%" PRIu32 "] DATA processing next fragment of %s frame %" PRIu32 ", index: %" PRIu64 ", len: %" PRIu32 "", _server->url(), _clientId,
           (_pinfo.message_opcode == WS_TEXT) ? "text" : "binary", _pinfo.num, _pinfo.index, (uint32_t)datalen
         );
-        _handleDataEvent(data, datalen, datalen == plen);  // datalen == plen means that we are processing the last part of the current TCP packet
+        if (!_handleDataEvent(data, datalen, datalen == plen)) {  // datalen == plen means that we are processing the last part of the current TCP packet
+          return;                                                 // stop processing on failure
+        }
       }
 
       // track index for next fragment
@@ -704,6 +707,7 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
           if (_client) {
             _client->close();
           }
+          return;  // our object is now destroyed, so we must return immediately to avoid accessing any member
         } else {
           _status = WS_DISCONNECTING;
           if (_client) {
@@ -729,7 +733,9 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
           (_pinfo.message_opcode == WS_TEXT) ? "text" : "binary", _pinfo.num, _pinfo.index, (uint32_t)datalen
         );
 
-        _handleDataEvent(data, datalen, datalen == plen);  // datalen == plen means that we are processing the last part of the current TCP packet
+        if (!_handleDataEvent(data, datalen, datalen == plen)) {  // datalen == plen means that we are processing the last part of the current TCP packet
+          return;                                                 // stop processing on failure
+        }
 
         if (_pinfo.final) {
           _pinfo.num = 0;
@@ -759,7 +765,7 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
   }
 }
 
-void AsyncWebSocketClient::_handleDataEvent(uint8_t *data, size_t len, bool endOfPaquet) {
+bool AsyncWebSocketClient::_handleDataEvent(uint8_t *data, size_t len, bool endOfPaquet) {
   // ------------------------------------------------------------
   // Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
   // Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
@@ -790,9 +796,11 @@ void AsyncWebSocketClient::_handleDataEvent(uint8_t *data, size_t len, bool endO
         _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy.get(), len);
       } else {
         async_ws_log_e("Failed to allocate");
-        if (_client) {
-          _client->abort();
+        AsyncClient *c = _client;
+        if (c) {
+          c->abort();
         }
+        return false;  // failure!
       }
     } else {
       uint8_t backup = data[len];
@@ -803,6 +811,7 @@ void AsyncWebSocketClient::_handleDataEvent(uint8_t *data, size_t len, bool endO
   } else {
     _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, len);
   }
+  return true;
 }
 
 size_t AsyncWebSocketClient::printf(const char *format, ...) {
@@ -997,11 +1006,13 @@ void AsyncWebSocket::_handleEvent(AsyncWebSocketClient *client, AwsEventType typ
 
 AsyncWebSocketClient *AsyncWebSocket::_newClient(AsyncWebServerRequest *request) {
   asyncsrv::lock_guard_type lock(_ws_clients_lock);
-  _clients.emplace_back(request, this);
+  // Hold the request in scope for the user callback to inspect
+  std::shared_ptr<AsyncWebServerRequest> req_lock = request->shared_from_this();
+  // Adopt the client object from the request
+  _clients.emplace_back(request->clientRelease(), this);
   // 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;
+  // req_lock releases the request object at the end of this function scope
   return &_clients.back();
 }
 

src/AsyncWebSocket.h

@@ -235,20 +235,13 @@ class AsyncWebSocketClient {
   void _clearQueue();
 
   // this function is called when a text message is received, in order to copy the buffer and place a null terminator at the end of the buffer for easier handling of text messages.
-  void _handleDataEvent(uint8_t *data, size_t len, bool endOfPaquet);
+  // Returns true on success, false on failure (e.g. memory allocation failure)
+  bool _handleDataEvent(uint8_t *data, size_t len, bool endOfPaquet);
 
 public:
   void *_tempObject;
 
   AsyncWebSocketClient(AsyncClient *client, AsyncWebSocket *server);
-
-  /**
-   * @brief Construct a new Async Web Socket Client object
-   * @note constructor would take the ownership of of AsyncTCP's client pointer from `request` parameter and call delete on it!
-   * @param request
-   * @param server
-   */
-  AsyncWebSocketClient(AsyncWebServerRequest *request, AsyncWebSocket *server) : AsyncWebSocketClient(request->clientRelease(), server){};
   ~AsyncWebSocketClient();
 
   // client id increments for the given server

src/ESPAsyncWebServer.h

@@ -508,13 +508,28 @@ 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<AsyncWebServerRequest> create(AsyncWebServer *server, AsyncClient *client) {
+    AsyncWebServerRequest *req = new (std::nothrow) AsyncWebServerRequest(server, client);
+    if (req) {
+      req->_this = std::shared_ptr<AsyncWebServerRequest>(req);  // store shared pointer to this request
+      return req->_this;
+    }
+    return {};  // empty shared_ptr
+  }
   ~AsyncWebServerRequest();
 
+  std::shared_ptr<AsyncWebServerRequest> shared_from_this() {
+    return _this;
+  }
+
   AsyncClient *client() {
     return _client;
   }
@@ -524,6 +539,8 @@ class AsyncWebServerRequest {
    * AsyncClient pointer will be abandoned in this instance,
    * the further ownership of the connection should be managed out of request's life-time scope
    * could be used for long lived connection like SSE or WebSockets
+   * This causes the request object to self-destruct; make sure you're holding a shared_ptr if
+   * you need to keep it alive any longer (see shared_from_this())
    * @note do not call this method unless you know what you are doing, otherwise it may lead to
    * memory leaks and connections lingering
    *
@@ -1763,7 +1780,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);
 };

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<AsyncWebServerRequest> 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<AsyncWebServerRequest> 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<AsyncWebServerRequest> 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,12 @@ void AsyncWebServerRequest::_onDisconnect() {
   if (_onDisconnectfn) {
     _onDisconnectfn();
   }
-  _server->_handleDisconnect(this);
+  // Pull shared pointer on to this context
+  // This will induce self-destruction if there are no other active handles
+  // This is safe to call here as it is the last operation in this callback
+  // We move to a stack local before destruction a simple reset would attempt to
+  // write through the now-destroyed object.
+  std::shared_ptr<AsyncWebServerRequest> self = std::move(_this);
 }
 
 void AsyncWebServerRequest::_addGetParams(const String &params) {
@@ -1054,9 +1061,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<AsyncWebServerRequest>(this, doNotDelete);
   _paused = true;
   return _this;
 }
@@ -1065,7 +1069,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 +1330,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 +1356,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;
@@ -1469,6 +1476,17 @@ bool AsyncWebServerRequest::isExpectedRequestedConnType(RequestedConnectionType
 AsyncClient *AsyncWebServerRequest::clientRelease() {
   AsyncClient *c = _client;
   _client = nullptr;
+  // Ensure the client object no longer refers to us
+  c->onError({}, nullptr);
+  c->onAck({}, nullptr);
+  c->onDisconnect({}, nullptr);
+  c->onTimeout({}, nullptr);
+  c->onData({}, nullptr);
+  c->onPoll({}, nullptr);
+  // Now that we are no longer bound to the client, self-destruct at the earliest opportunity by moving the shared pointer to a local.
+  // This will decrement the reference count and delete the object when this function ends if there are no other references.
+  // If this function is called by an _onAck handler or the like, the local lock will keep the request object in scope until the handler returns, preventing a use-after-free.
+  std::shared_ptr<AsyncWebServerRequest> destroy_self = std::move(_this);
   return c;
 }
 

src/WebServer.cpp

@@ -4,6 +4,7 @@
 #include "ESPAsyncWebServer.h"
 #include "WebHandlerImpl.h"
 
+#include <memory>
 #include <string>
 #include <utility>
 
@@ -47,8 +48,8 @@ AsyncWebServer::AsyncWebServer(uint16_t port) : _server(port) {
         return;
       }
       c->setRxTimeout(3);
-      AsyncWebServerRequest *r = new AsyncWebServerRequest((AsyncWebServer *)s, c);
-      if (r == NULL) {
+      std::shared_ptr<AsyncWebServerRequest> r = AsyncWebServerRequest::create(static_cast<AsyncWebServer *>(s), c);
+      if (!r) {
         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)