Skip to content

Fix safety issues with AsyncClient destruction in user callbacks - #123

Open
willmmiles wants to merge 5 commits into
mainfrom
close-in-callback-safety
Open

Fix safety issues with AsyncClient destruction in user callbacks#123
willmmiles wants to merge 5 commits into
mainfrom
close-in-callback-safety

Conversation

@willmmiles

Copy link
Copy Markdown

Ensure no use-after-frees occur if the AsyncClient object is destroyed while in a user callback. This guarantees correct behavior if user code calls close() in an event callback (such as onData, onPacket, onTimeout, etc.) and frees the AsyncClient in the onDisconnect() callback.

Note that this does not insulate the user callback code itself against internal use-after-free events, such as using a captured lambda value after destroying the AsyncClient and thus the std::function storing the capture. Any user code that could destruct its own AsyncClient must be cognizant of the risks. We may want to consider an explicit "deferred close" API to make it easier for clients to reason about their own code.

Includes a couple of other little tweaks:

  • Removed duplicate #include (probably a merge issue somewhere earlier)
  • Ensure that _rx_ack_len is initialized

Ensure that it is safe to destruct an AsyncClient from any user
callback.   Fixes a guaranteed use-after-free if `close()` was called
inside `onData()`.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds callback-lifetime tracking to prevent AsyncClient use-after-free during user callbacks.

Changes:

  • Tracks client validity across nested callbacks.
  • Safely stops receive processing after closure/destruction.
  • Initializes _rx_ack_len and removes a duplicate include.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/AsyncTCP.h Adds callback context state and moves ackLater() implementation.
src/AsyncTCP.cpp Implements callback safety, receive cleanup, and initialization fixes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/AsyncTCP.cpp
// call to track if the object was destroyed. If so, we do not reset the pointer, which GCC identifies as a possible
// dangling reference case. It is not - the reference was destroyed along with the AsyncClient object.
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdangling-pointer"
Comment thread src/AsyncTCP.cpp Outdated
return ERR_OK;
}

// In LwIP Thread
@willmmiles

Copy link
Copy Markdown
Author

Arg. Upon re-re-review, I think there might still be something off with the close()-from-onData() semantics. If close() is invoked from an onData() callback, with this patch as-is we'll never ack that final packet (instead we sent TCP RST). This is not new but probably not the ideal behaviour.

willmmiles and others added 4 commits September 1, 2026 22:44
The old _lwip_fin function was entirely unreferenced.
Per @Copilot, it's only available in newer GCCs.
If we're closing as a response to some message from the remote end,
acknowledge the last processed pbuf first.  This ensures that we'll send
FINs instead of RSTs on correctly processed messages.
@willmmiles

Copy link
Copy Markdown
Author

Arg. Upon re-re-review, I think there might still be something off with the close()-from-onData() semantics. If close() is invoked from an onData() callback, with this patch as-is we'll never ack that final packet (instead we sent TCP RST). This is not new but probably not the ideal behaviour.

Updated with a fix for this. To be clear, the issue of incorrectly sending RSTs if we close() from onData() was not new to this branch, it's also a probem in main -- but since I'm in there updating the close handling semantics, might as well get them all straightened out at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants