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

[mathieucarbou]

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.

Ref discussions:

• fix(queue) use-after-free in abort() path AsyncTCP#118 (comment)
• fix(ws): Fully handle websocket frame tearing (closes #453) #462
• https://discord.com/channels/1330858863850946662/1330987584398164080/1529130711515271328
• https://discord.com/channels/1330858863850946662/1330860105440301066/1529142440382955653

[mathieucarbou]

Note: most of the issues fixes in this PR were also there before AsyncTCP 3.5.0 with regard to the synchronous call close().

[willmmiles]

This (the shared_ptr) is the way out of locking and could substantially clean up the management logic. If we use the shared_ptr to take responsibility for doing object cleanup, then it's easy to extend the object lifetime over a function: just take a copy of the shared_ptr on the stack.

[willmmiles]

I can sketch this later tonight.

[mathieucarbou]

Thanks!
Like I said above, the locking logic was isolated in a second commit since I also did not like it.
Feel free to drop it and complete the PR :-)
I will have a look tomorrow morning and merge the result if you are finished.

[mathieucarbou]

update: removed the second commit with the lock, as I asked the user who got the issue to retest with these changes, and he will do so in the morning.

[mathieucarbou]

Replaced by #465