fix(queue) use-after-free in abort() path

[mathieucarbou]

If aborting in WebSocket (torm frame), we must also purge the queue and reset callbacks

AsyncClient::_error(-13) execures with a LWIP_TCP_ERROR event still present in the queue but client was dereferenced before

Crash that was occurring:

First one seen:

[ 91238][V][AsyncTCP.cpp:1125] _poll(): onPoll took 8756 us
[ 91245][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 15160 us
[ 91252][V][AsyncWebSocket.cpp:608] _onError(): [/ws][1] ERROR -13
[ 91258][V][AsyncTCP.cpp:1033] _error(): onError took 6115 us
Guru Meditation Error: Core  1 panic'ed (IllegalInstruction). Exception was unhandled.
Core  1 register dump:
PC      : 0x7ec0eda1  PS      : 0x00060730  A0      : 0x001ad0f4  A1      : 0x3ffd20e0  
A2      : 0x3ffd2bac  A3      : 0xc00a8962  A4      : 0x3ffd2b94  A5      : 0x00000409  
A6      : 0x400d6866  A7      : 0x3ffd20cc  A8      : 0x800d64ae  A9      : 0x3ffd20d0  
A10     : 0x3ffd2bac  A11     : 0x00000032  A12     : 0x3ffd20e8  A13     : 0x00000040  
A14     : 0x3ffd20e2  A15     : 0x3ffc3c7d  SAR     : 0x00000001  EXCCAUSE: 0x00000000  
EXCVADDR: 0x00000000  LBEG    : 0x40083439  LEND    : 0x40083441  LCOUNT  : 0x00000027  
Backtrace: 0x7ec0ed9e:0x3ffd20e0 |<-CORRUPTED
ELF file SHA256: 5255f2a65
Rebooting...

Second one:

[mathieucarbou]

I tested successfully that it solves the issue seen when abort() is called but lwip events are in the queue. See PR ESP32Async/ESPAsyncWebServer#454

[willmmiles]

IIRC this was intentionally omitted to avoid a different crash case. I'll review my notes and get back to you tonight.

[mathieucarbou]

IIRC this was intentionally omitted to avoid a different crash case. I'll review my notes and get back to you tonight.

Note: this is easy to reproduce with WebSocket but I think any call to abort() also in the HTTP code of espasyncws is susceptible to cause this crash.

[willmmiles]

There is a bug here, but I don't think this is the correct fix.

What I forgot was that the LwIP semantic for abort() is that the error callback is triggered -- so that's the call path we've been taking in AsyncTCP. This queues an event to call AsyncClient::_error, which does call the discard callback. I'm sorry - I was wrong, the currently shipping AsyncWebServer is fine and does not leak, ultimately _onDisconnect does get called!

I tested this with the current AWS main using curl -X GET -H "Content-Type: multipart/form-data" http://192.168.4.1/ to generate a header parsing abort; and the heap stats show that no memory is being lost.

However, as you've discovered, there is a bug with this call sequence:

client->abort();
delete client;

The client destructor does not correctly handle the case that the client object has been invalidated, but there remains a final error event in the queue.

We can potentially improve safety for this sequence by calling _remove_events_for_client() in the AsyncClient destructor if there's no _pcb bound, but this doesn't solve the problem in the general case. In particular, attempting that sequence on a third thread -- not the LwIP task or the AsyncTCP task, but a user task (say, a task trying to queue a websocket message, and deciding to pull the plug on a connection) runs a risk that the error callback was already dequeued by the AsyncTCP task, but then control was handed back to the user task and it processed the delete.

I don't think it's possible to close that hole without coming up with a stronger interlock for the destructor (and the attendant performance cost of taking and releasing the lock to indicate client-object-in-use for every event being processed). I had looked through this before -- I'd thought of a few different implementations (reference counting? condvars?) but in the end it seemed that "just don't do that" was the least evil course of action.

What do you think?

[mathieucarbou]

What do you think?

I think we need to fix it as much as we can because this issue is pretty easy to reproduce in ESP32Async/ESPAsyncWebServer#454 with the WebSocket example: we just need to wait a little time / or get some torn frames. So I guess any other path were abort() is called is also susceptible to cause this crash.

If I understand, the real issue is that an event is queued. If an abort() occurs, this is not an error: this is either an expected user interaction, or an expected library interaction following a wrong request / frame, leading to an RST.

_error would be more suited for unexpected situations like network disruption.

If abort is called, I would rather see the queue cleared, callbacks cleared, basically everything cleared ASAP to reclaim memory and avoid use after free. abort is a terminal operation.

[willmmiles]

That's a major and breaking change in the API contract. Currently, anywhere abort() is called, the error and dispose callbacks are executed on the AsyncTCP thread.

This patch removes that, so those callbacks are no longer run.

[willmmiles]

The fix for ESP32Async/ESPAsyncWebServer#454 is to remove the explicit onDisconnect() calls after abort. They are not necessary - abort() will call the onDisconnect callback. I was wrong to suggest it did not.

[willmmiles]

Sorry to keep spamming you..

If I understand, the real issue is that an event is queued. If an abort() occurs, this is not an error: this is either an expected user interaction, or an expected library interaction following a wrong request / frame, leading to an RST.

The event that's being queued is the cleanup event! It's not caused by any external source.

[mathieucarbou]

@willmmiles : why not taking another approach: the problem is that there is a use after free caused by the event happening after. So in that case why not completing this PR and call the _discard_cb in int8_t AsyncClient::abort() exactly like it is done for close ? I have updated this PR to reflect that. This is similar code that I had previously raised in another PR.

• abort() and close() -> calls discard_cb like before (but not from within the async_tcp even queue)
• abort() -> does not call error cb anymore, which was a bug IMO: aborting is not an unexpected error but a user driven call, like close. A connection drop for exemple is an unexpected error.

Aligning abort with close makes also sense because abort is a sort of abrupt close driven by a user call.

=> Test with curl https://192.168.4.1:80/ (to cause an abort call from request processing)

AsyncTCP (main) with ESPAsyncWebServer (main)

[221871][V][AsyncTCP.cpp:1602] _accepted(): onClient took 601 us
[221877][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 6611 us
[221908][D][WebRequest.cpp:130] _onData(): SSL/TLS handshake detected: resetting connection
[221917][V][AsyncTCP.cpp:1080] _recv(): onData took 9755 us
[221923][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 15318 us
[221931][V][AsyncTCP.cpp:1033] _error(): onError took 22 us
[221936][V][AsyncTCP.cpp:1036] _error(): onDisconnect took 188 us
[221942][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 11478 us

AsyncTCP (fix/abort - this PR) with ESPAsyncWebServer (main)

[ 34744][V][AsyncTCP.cpp:1616] _accepted(): onClient took 648 us
[ 34750][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 6633 us
[ 34757][D][WebRequest.cpp:130] _onData(): SSL/TLS handshake detected: resetting connection
[ 34767][V][AsyncTCP.cpp:955] abort(): onDisconnect took 310 us
[ 34773][V][AsyncTCP.cpp:1094] _recv(): onData took 16198 us
[ 34779][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 21724 us

Yes this is a change in behaviour, I agree, but to me it is more a fix than anything else. The fact that one action is calling 2 callbacks is a mistake because there is no way to determine from the error callback if the error is unexpected or not because it comes from a wanted user action (abort).

[willmmiles]

@willmmiles : why not taking another approach: the problem is that there is a use after free caused by the event happening after. So in that case why not completing this PR and call the _discard_cb in int8_t AsyncClient::abort() exactly like it is done for close ? I have updated this PR to reflect that. This is similar code that I had previously raised in another PR.

Right - for context: #79, #80, #81, #82

Broadly, my thoughts are:

• LwIP runs its error callback when the client calls abort(); historically, this library has always done so too. I think we should preserve this. It's maybe not the ideal semantic in a clean-sheet design, but I don't think we should change this one now, especially since there are at least two other "API-compatible" libraries in orbit here today (I'm thinking of the ESP8266 and RP versions; there could be more).
• I wrote Restore abort dispose semantics #82 with the idea that we wanted to preserve the task semantics of previous versions of the library, ie. in case of an abort(), those callbacks must be run on the AsyncTCP task. As we've clarified today, that old approach has a safety hole that's difficult to solve. As I recall things, we'd considered both approaches at the time and elected to make the safety tradeoff to preserve strict compatibility with the behaviour of previous versions.

I'm OK with moving the callback execution to the abort()ing task context. We'll want to make sure that's explicitly called out in the release notes as it is an API change that could bite some users. (Alas, the perennial curse of async libraries is users having to pay a lot of attention as to what context a given callback might be run in. Somebody might be relying on a guarantee that their dispose callback will always be run on the AsyncTCP task.)

I do think we should run the whole AsyncClient::_error() for compatibility, though.

Lastly: there'll still be a race in the case where a user task calls abort() on a client object where an error event had been dequeued by the AsyncTCP task but not yet executed. Or in other words: changing the callback context won't fix the problem completely -- we're trading off a change in client API context to reduce the likelihood. I can think of a dozen approaches to do better, but they all come at the cost of essentially having to lock over any AsyncClient call to ensure meaningful serialization between callbacks and user threads.

[mathieucarbou]

I do think we should run the whole AsyncClient::_error() for compatibility, though.

I am OK with that if we account for the other libs and backward compatible reasons, indeed this is a bigger design change.

I updated the PR to add the call to the error callback just before the disconnect:

int8_t AsyncClient::abort() {
  int8_t err = _tcp_abort(&_pcb, this);
  // _pcb is now NULL
  async_tcp_log_elapsed("onError", _error_cb(_error_cb_arg, this, err));
  // Like close(), notify the discard callback when an abort actually
  // happened (ERR_ABRT) or a queued error event was purged (ERR_OK). When the
  // pcb was already gone and nothing was queued (ERR_CONN), no disconnect
  // occurred and the discard callback must not be invoked.
  if ((err != ERR_CONN) && _discard_cb) {
    async_tcp_log_elapsed("onDisconnect", _discard_cb(_discard_cb_arg, this));
  }
  return err;
}

AsyncTCP (fix/abort - this PR) with ESPAsyncWebServer (main)

[ 27923][V][AsyncTCP.cpp:1617] _accepted(): onClient took 626 us
[ 27929][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 6626 us
[ 27936][D][WebRequest.cpp:130] _onData(): SSL/TLS handshake detected: resetting connection
[ 27946][V][AsyncTCP.cpp:950] abort(): onError took 53 us
[ 27951][V][AsyncTCP.cpp:956] abort(): onDisconnect took 160 us
[ 27957][V][AsyncTCP.cpp:1095] _recv(): onData took 20873 us
[ 27963][V][AsyncTCP.cpp:340] _async_service_task(): handle_async_event took 26411 us

Right now, with the current state, ESP32Async/ESPAsyncWebServer#454 is crashing in a few seconds with the websocket example because of this use after free.

[willmmiles]

I updated the PR to add the call to the error callback just before the disconnect:

int8_t AsyncClient::abort() {
  int8_t err = _tcp_abort(&_pcb, this);
  // _pcb is now NULL
  async_tcp_log_elapsed("onError", _error_cb(_error_cb_arg, this, err));
  // Like close(), notify the discard callback when an abort actually
  // happened (ERR_ABRT) or a queued error event was purged (ERR_OK). When the
  // pcb was already gone and nothing was queued (ERR_CONN), no disconnect
  // occurred and the discard callback must not be invoked.
  if ((err != ERR_CONN) && _discard_cb) {
    async_tcp_log_elapsed("onDisconnect", _discard_cb(_discard_cb_arg, this));
  }
  return err;
}

The error callback should only be run if we actually executed the abort or consumed the error event (same as the dispose), so we can leverage the existing _error() handler.

int8_t AsyncClient::abort() {
  int8_t err = _tcp_abort(&_pcb, this);
  // _pcb is now NULL
  // LwIP invokes the error callback when abort is issued; preserve this semantic.
  // This will also trigger the dispose callback.  
  // If the pcb was previously invalidated by some other queued error, we've discarded that value; so we always send ERR_ABRT.
  if ((err != ERR_CONN)) {
    _error(ERR_ABRT);
  }
  return err;
}

Right now, with the current state, ESP32Async/ESPAsyncWebServer#454 is crashing in a few seconds with the websocket example because of this use after free.

Right, the previous iterations were manually calling the onDisconnect(). I'm so sorry for the noise on that one, that's entirely on me. :( I see you've got it cleaned up now.

[mathieucarbou]

I'm OK with moving the callback execution to the abort()ing task context. We'll want to make sure that's explicitly called out in the release notes as it is an API change that could bite some users. (Alas, the perennial curse of async libraries is users having to pay a lot of attention as to what context a given callback might be run in. Somebody might be relying on a guarantee that their dispose callback will always be run on the AsyncTCP task.)

Yes I agree... The expectation form where the code is run is different. But we already have different sources for teh discard_cb: it could be run already from the async_tcp task (poll) or user code (close). So anyway currently the user could not rely on only 1 thread for discard_cb.

The only change is for _error_cb, which, now, might be run from caller context. And I don't think this one is problematic. As an error handler, there are 80% chances that if implemented it has to deal with something out of scope of the async_tcp task in order to report the error or do something. So for such use cases, the code has better to be run from outside the async_tcp task anyway.

The error callback should only be run if we actually executed the abort or consumed the error event (same as the dispose), so we can leverage the existing _error() handler.

Updated. Thanks! This part was not clear to me...

Right, the previous iterations were manually calling the onDisconnect(). I'm so sorry for the noise on that one, that's entirely on me. :( I see you've got it cleaned up now.

Yes I have updated ESP32Async/ESPAsyncWebServer#454 and tested it with and without this PR. It runs fine now.

[willmmiles]

So anyway currently the user could not rely on only 1 thread for discard_cb.

Well, until this PR, one could guarantee that the discard would run on the AsyncTCP task ... if-and-only-if one never called close() from a user task. Hyrum's law is the library maintainers curse.

I'm maybe overly sensitive to this kind of detail because too often I am guilty of writing code that leverages that kind of "guarantee", sadly. I'm reminded that I should review my request queue middleware WIP... I'd had the AI sketch it with an eye on moving WLED to the upstream AWS instead of our archaic fork, but haven't looked at it in some months. There were some tricky bits to the orchestration IIRC. Maybe I'll get to it in the fall...

[mathieucarbou]

Well, until this PR, one could guarantee that the discard would run on the AsyncTCP task ... if-and-only-if one never called close() from a user task. Hyrum's law is the library maintainers curse.

And Is there a reason why the close() cannot raise an event and have the discard callback called through the async task also for close ?

For now there is no guarantee from where the discard cb is called.

I understand also that such guarantee has some value…

I would be opened (in fact I would prefer) if we could provide such guarantee that any callback is called from the async task, error, abort, close included. No exception when we call close.

but right now there is no guarantee at all for the discard cb that can be called from anywhere already and this pr also applies the same rules for the error cb for abort.

also the problem of relying on the queue is that it can introduce a delay and a potential loss of the event.

Close and abort are linked to resource cleaning so they must be executed fast and their execution must be guaranteed I think.

so if we have their event moved onto a queue to guarantee the execution of the callbacks from the async tcp task, then, how we can guarantee that they are executed with higher priority but also how we can guarantee that they can be queued… ?

I don’t have the answers.

[willmmiles]

I don’t have the answers.

I think you understand the problem well -- particularly for resource cleanup, there's a big tradeoff to be made for immediate vs deferred execution. I don't think there's a one-size-fits-all answer.

From an API perspective, doing immediate teardown feels like the more natural semantic to me. That said, I think deferral should be allowed -- the usual approach is to provide a way to queue an arbitrary function for later execution on the task, so users could schedule those teardowns how they like (or any other work, really, that needs to share a task to avoid excessive locking).

The best possible answer IMO is to abstract over the AsyncTCP handler task, giving users maximum possible control. Make the event queue an explicitly instantiable object, so client objects can potentially be bound to different tasks and serviced when the user likes. On each queue, support an API that allows both queuing an arbitrary user std::function, as well as a "process one event" call. Finally, provide a convenience function that makes a task that loops calling that function. This allows users to directly control execution however they like, at minimal to no cost for the simple cases -- we can instantiate a static internal queue and task for AsyncClient and AsyncServers if no queue is explicitly supplied by the user.

Still it's a bunch of work for a hard-to-predict reward -- I don't really know how wide the user base of this library is (AsyncWebServer notwithstanding).