fix(http): prevent use-after-free from synchronous abort/close() - v2#466
Closed
mathieucarbou wants to merge 2 commits into
Closed
fix(http): prevent use-after-free from synchronous abort/close() - v2#466mathieucarbou wants to merge 2 commits into
mathieucarbou wants to merge 2 commits into
Conversation
…ncTCP 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.
…ared_ptr lifecycle
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<AsyncWebParameter> 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<AsyncWebServerRequest> 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.
mathieucarbou
changed the title
willmmiles
requested changes
Jul 22, 2026
| } | ||
|
|
||
| void AsyncWebServerRequest::_onTimeout(uint32_t time) { | ||
| std::shared_ptr<AsyncWebServerRequest> self = _this; // Ensure we stay in scope over the function |
There was a problem hiding this comment.
This is a waste of resources and should be omitted as it is not necessary for this function.
Comment on lines
+284
to
+293
| // 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<AsyncWebServerRequest> 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 |
There was a problem hiding this comment.
This is incorrect and unnecessary. Sorry I have not had a chance to post the detailed explanation yet.
Comment on lines
-249
to
-255
| std::shared_ptr<AsyncWebServerRequest> self = _this; // Ensure we stay in scope over the function | ||
| // os_printf("a:%u:%u\n", len, time); | ||
| if (!_response) { | ||
| return; | ||
| } | ||
|
|
||
| std::shared_ptr<AsyncWebServerRequest> self = _this; // Ensure we stay in scope over the function | ||
|
|
There was a problem hiding this comment.
This was intentional to avoid the cost of unnecessarily locking.
mathieucarbou
marked this pull request as draft
July 22, 2026 13:11
Member
Author
|
Closing this PR: it was only raised to allow a user to test the fixes introduced in the second commit. PR #465 is the original one and @willmmiles will bring the necessary fixes into it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Same as #465 but a commit was added for further fixes to be able to test the result.