Skip to content

Error/closing stability #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mathieucarbou merged 4 commits into from
Jul 30, 2025
Merged

Error/closing stability #79

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc3f80a
Check event queue on null close
willmmiles Jul 27, 2025
f2ec361
Additional close semantic fixes
willmmiles Jul 30, 2025
97c6772
Fix use-after-free in tcp_error
willmmiles Jul 30, 2025
f736219
Ensure dispose is run in close if needed
willmmiles Jul 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 36 additions & 19 deletions src/AsyncTCP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ static inline lwip_tcp_event_packet_t *_get_async_event() {
}
}

static void _remove_events_for_client(AsyncClient *client) {
static size_t _remove_events_for_client(AsyncClient *client) {
lwip_tcp_event_packet_t *removed_event_chain;
{
queue_mutex_guard guard;
Expand All @@ -269,6 +269,7 @@ static void _remove_events_for_client(AsyncClient *client) {
removed_event_chain = t->next;
_free_event(t);
}
return count;
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The variable 'count' is being returned but is never declared or initialized in the function. This will cause a compilation error.

Copilot uses AI. Check for mistakes.
};

void AsyncTCP_detail::handle_async_event(lwip_tcp_event_packet_t *e) {
Expand Down Expand Up @@ -484,7 +485,8 @@ void AsyncTCP_detail::tcp_error(void *arg, int8_t err) {
// ets_printf("+E: 0x%08x\n", arg);
AsyncClient *client = reinterpret_cast<AsyncClient *>(arg);
if (client && client->_pcb) {
_reset_tcp_callbacks(client->_pcb, client);
// The pcb has already been freed by LwIP; do not attempt to clear the callbacks!
_remove_events_for_client(client);
client->_pcb = nullptr;
}

Expand Down Expand Up @@ -615,26 +617,35 @@ static esp_err_t _tcp_recved(tcp_pcb **pcb, size_t len) {
}

static err_t _tcp_close_api(struct tcpip_api_call_data *api_call_msg) {
// Unlike the other calls, this is not a direct wrapper of the LwIP function;
// we perform the AsyncClient teardown interlocked safely with the LwIP task.

// As a postcondition, the queue must not have any events referencing
// the AsyncClient in api_call_msg->close. This is because it is possible for
// an error event to have been queued, clearing the pcb*, but after the async
// thread has committed to closing/destructing the AsyncClient object.

tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg;
msg->err = ERR_CONN;
if (*msg->pcb) {
// Unlike the other calls, this is not a direct wrapper of the LwIP function;
// we perform the AsyncClient teardown interlocked safely with the LwIP task.
tcp_pcb *pcb = *msg->pcb;
_reset_tcp_callbacks(pcb, msg->close);
msg->err = tcp_close(pcb);
if (msg->err != ERR_OK) {
if (tcp_close(pcb) != ERR_OK) {
// We do not permit failure here: abandon the pcb anyways.
tcp_abort(pcb);
}
msg->err = ERR_OK;
*msg->pcb = nullptr; // PCB is now the property of LwIP
} else {
// Ensure there is not an error event queued for this client
if (_remove_events_for_client(msg->close)) {
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The function _remove_events_for_client now returns a size_t (count), but this condition will be true for any non-zero count. Consider being explicit about what you're checking for, such as 'if (_remove_events_for_client(msg->close) > 0)' for clarity.

Suggested change
if (_remove_events_for_client(msg->close)) {
if (_remove_events_for_client(msg->close) > 0) {

Copilot uses AI. Check for mistakes.
msg->err = ERR_OK; // dispose needs to be run
}
}
return msg->err;
}

static esp_err_t _tcp_close(tcp_pcb **pcb, AsyncClient *client) {
if (!pcb || !*pcb) {
return ERR_CONN;
}
tcp_api_call_t msg;
msg.pcb = pcb;
msg.close = client;
Expand All @@ -643,21 +654,29 @@ static esp_err_t _tcp_close(tcp_pcb **pcb, AsyncClient *client) {
}

static err_t _tcp_abort_api(struct tcpip_api_call_data *api_call_msg) {
// Like close(), we must ensure that the queue is cleared
tcp_api_call_t *msg = (tcp_api_call_t *)api_call_msg;
msg->err = ERR_CONN;
if (*msg->pcb) {
tcp_abort(*msg->pcb);
tcp_pcb *pcb = *msg->pcb;
_reset_tcp_callbacks(pcb, msg->close);
tcp_abort(pcb);
*msg->pcb = nullptr; // PCB is now the property of LwIP
msg->err = ERR_OK;
} else {
// Ensure there is not an error event queued for this client
_remove_events_for_client(msg->close);
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The return value of _remove_events_for_client is ignored here, unlike in the close API where it's used to determine if dispose should be run. Consider documenting why the return value is not needed in the abort case or handle it consistently.

Suggested change
_remove_events_for_client(msg->close);
if (_remove_events_for_client(msg->close)) {
msg->err = ERR_OK; // dispose needs to be run
}

Copilot uses AI. Check for mistakes.
}
return msg->err;
}

static esp_err_t _tcp_abort(tcp_pcb **pcb) {
static esp_err_t _tcp_abort(tcp_pcb **pcb, AsyncClient *client) {
if (!pcb || !*pcb) {
return ERR_CONN;
}
tcp_api_call_t msg;
msg.pcb = pcb;
msg.close = client;
tcpip_api_call(_tcp_abort_api, (struct tcpip_api_call_data *)&msg);
return msg.err;
}
Expand Down Expand Up @@ -903,7 +922,7 @@ void AsyncClient::close(bool now) {

int8_t AsyncClient::abort() {
if (_pcb) {
_tcp_abort(&_pcb);
_tcp_abort(&_pcb, this);
// _pcb is now NULL
}
return ERR_ABRT;
Expand Down Expand Up @@ -968,13 +987,11 @@ void AsyncClient::ackPacket(struct pbuf *pb) {

int8_t AsyncClient::_close() {
// ets_printf("X: 0x%08x\n", (uint32_t)this);
int8_t err = ERR_OK;
if (_pcb) {
_tcp_close(&_pcb, this);
// _pcb is now NULL
if (_discard_cb) {
_discard_cb(_discard_cb_arg, this);
}
int8_t err = _tcp_close(&_pcb, this);
// _pcb is now NULL
if ((err == ERR_OK) && _discard_cb) {
// _pcb was closed here
_discard_cb(_discard_cb_arg, this);
}
return err;
}
Expand Down