fix(ws): Prevent torn frames when add() fails after the frame header (closes #453)

[inF1704]

Fixes #453

Makes WebSocket frames all-or-nothing in webSocketSendFrame() as discussed in the issue:

• partial add() after the header was committed → client->abort() (RST is
  recoverable, a torn frame stream is not)
• a failing client->send() is no longer treated as an error — the frame is
  already queued in lwIP and flushes on the next poll/ack

Field-tested on ESP32-S3 (2 concurrent clients, large bursts): the
"Received start of new message but previous message is unfinished" /
1006 reconnect storms are gone; under extreme lwIP pressure connections
now reset cleanly and reconnect instead of corrupting.

[mathieucarbou]

I am approving the changes: they make sense to me, although not tested yet (I will be able to test this weekend).

@inF1704 : you confirm having tested the fix ? sorry - re-read the description ;-) perfect!

@willmmiles @me-no-dev could you please have a look ?

I am thinking that we could merge this fix sonner than later in main, but wait a week before releasing (I would like to test before). Having it merged sooner than later in main will allow people pointing to main to grab the fix and report any issue also. For this kind if fix that is harder to test, this is an advantage.

Thanks!

[willmmiles]

I don't think I'd qualify this approach as "fix" so much as a "stopgap". I'm familiar with the underlying problem; it was pretty easy to replicate on ESP8266. The key insight is that space() returns the available TCP window headroom; it is not an indication of whether or not there's enough contiguous heap to allocate another packet buffer to use that headroom.

On a review basis: I don't think it's safe to call client->abort() in this context. (Yes, the code already does that - that doesn't make it correct.) AsyncClient::abort() does not run the onDisconnect() callback, so the underlying data structures will never be cleaned up and the client object memory would be stuck forever. The alternative of calling close() is even worse: it would destruct the AsyncWebSocketClient in use on the stack, resulting in a use-after-free of the queue data structures.

I've got an unfinished attempt at WebSocket cleanup in my workspace from back in January, but I never got around to finishing it -- in fact it was this very issue that was the point where I stopped. If we want to take a "failed send closes the connection" approach, the error state will need to be propagated upwards on the stack until it can be handled safely. This in practice means a function signature change: the various send() calls have to be able to differentiate "failure" from "busy, try again later" -- or in other words, we need a -1 to go with 0 as a return code. That's a pretty aggressive change.

Upon review, I think we don't really have much choice but to adopt a real state machine to track partially sent messages. At the time I was working on it, I couldn't shake the feeling it was a lot of extra bookkeeping to handle an edge case. I don't think there's any safe way around it though -- in practice there is no way to validate in advance if an add() will succeed short of trying it, so client code just has to be prepared to handle partial add()s.

[mathieucarbou]

On a review basis: I don't think it's safe to call client->abort() in this context. (Yes, the code already does that - that doesn't make it correct.) AsyncClient::abort() does not run the onDisconnect() callback, so the underlying data structures will never be cleaned up and the client object memory would be stuck forever.

I never noticed we had this issue indeed!

Upon review, I think we don't really have much choice but to adopt a real state machine to track partially sent messages. At the time I was working on it, I couldn't shake the feeling it was a lot of extra bookkeeping to handle an edge case. I don't think there's any safe way around it though

I agree, but this is a major rework.

This PR uses abort() and like you said abort() is already used so this does not change the effect (missed cleanup).

So what about this plan:

• we could merge this "fix" considering its solves the problem (not making things worse)
• we could then in parallel work on a PR / proposal to introduce a way to correctly cleanup when abort is called - this is the missing piece we need anyway, whether this PR is merged or not
• and on the long run, take the time to refactor this websocket implementation ?

Note: the abort() issue not calling onDisconnect is at about 30 places in the code so this to me another kind of fix out of the scope of this PR.

[mathieucarbou]

@willmmiles could AsyncTCP simply be fixed as such to call the discard_cb on abort() ?

=> ESP32Async/AsyncTCP#116

that would be similar to what's done for error:

void AsyncClient::_error(int8_t err) {
  if (_error_cb) {
    _error_cb(_error_cb_arg, this, err);
  }
  if (_discard_cb) {
    _discard_cb(_discard_cb_arg, this);
  }
}

edit

just saw that on ESP8266, abort() does not call the callback also... So fixing AsyncTCP is not a complete solution.

[mathieucarbou]

@willmmiles : I ended up you way and I created a draft PR (#455) based on @inF1704's commit added a commit to add a refactoring regarding the abort() calls that we also have .

This is an untested draft PR just to discuss the idea, and also to again discuss the plan above: why not merging this and fix the abort() calls in another PR, since this is another issue.

[willmmiles]

On a review basis: I don't think it's safe to call client->abort() in this context. (Yes, the code already does that - that doesn't make it correct.) AsyncClient::abort() does not run the onDisconnect() callback, so the underlying data structures will never be cleaned up and the client object memory would be stuck forever.

I never noticed we had this issue indeed!

IIRC a lot of the WebRequest error paths used to call AsyncClient::close() instead of AsyncClient::abort(). Sorry I missed that it had changed!

Upon review, I think we don't really have much choice but to adopt a real state machine to track partially sent messages. At the time I was working on it, I couldn't shake the feeling it was a lot of extra bookkeeping to handle an edge case. I don't think there's any safe way around it though

I agree, but this is a major rework.

Well it's a good thing it's already half finished then ... ;)

@willmmiles could AsyncTCP simply be fixed as such to call the discard_cb on abort() ?
just saw that on ESP8266, abort() does not call the callback also... So fixing AsyncTCP is not a complete solution.

Yes. That's been the standard semantic since the beginning - I don't think we should change it.

Note: the abort() issue not calling onDisconnect is at about 30 places in the code so this to me another kind of fix out of the scope of this PR.

I ended up you way and I created a draft PR (#455) based on @inF1704's commit to add a refactoring regarding the abort() calls that we also have .

This is an untested draft PR just to discuss the idea, and also to again discuss the plan above: why not merging this and fix the abort() calls in another PR, since this is another issue.

Glad to see there there weren't too many abort() leaks in the rest of the codebase. If you want to PR the WebHanders.cpp and WebRequests.cpp fixes separately, that's an easy win. A quick test case would be to deliberately send null bytes in the request headers; it'll trip a parse failure and call abort().

Regarding websockets, I think it's mostly a matter of timeline. My process is still largely interrupt driven - I'm happy to dust off the WIP and finish it up over the next few days, but I can't commit to much faster than that. Let me know.

[mathieucarbou]

@willmmiles : ok let's do that. I will be able to resume this week-end + next week.

I will PR the non websocket part separately => this will be a quick merge.

And you'll be able to advance in your refactoring.

At that point, we will see whether we can merge your refactoring or if it is better to merge #454 #455 first, which keeps the current implementation for now but fixes the leak. The advantage I see for this is that there is less surprises / unknowns bugs that could happen.

[mathieucarbou]

@inF1704 : I added a commit to correctly handle the abort() of the WS channel and call onDisconnect after => would you be able to test again ?

@willmmiles : I have extracted the code related to the HTTP part in #458, which can be merged independently.

@willmmiles : For the next step... Since a complete WebSocket refactoring is more exhausting in terms of testing, and would probably sit in main for a longer time before being released, what about rather getting this fix merged and released before ? What do you think ?

[mathieucarbou]

I tested this PR and was able to trigger some torn fames and abort() calls by sending bursts.
But I got a crash because AsyncTCP abort() is not clearing callbacks and lwip queue, which I fixed in PR ESP32Async/AsyncTCP#118.

[inF1704]

Thanks for picking this up! I can't run a dedicated test setup right now, but some field data that may help: we've been running the functional equivalent of this PR (your #455 draft ported into our vendored copy, incl. the #458 HTTP part) on ESP32-S3 in the field since July 15. No torn frames, no crashes, reconnects are clean. However, this is with stock AsyncTCP 3.4.10, i.e. without your #118 fix. So far the abort path simply hasn't been hit in normal operation (torn frames became rare for us after we reduced WS queue pressure app-side). I'll switch back to the registry versions and retest once this and #118 are released, and will report back if anything comes up.

[mathieucarbou]

• Pending discussion here: fix(queue) use-after-free in abort() path AsyncTCP#118

[mathieucarbou]

@willmmiles : I updated last commit to correctly abort the websocket connection. Only c->abort() is necessary, onDisconnect cb is called then client freed.

I tested with AsyncTCP 3.4.10 and AsyncTCP (ESP32Async/AsyncTCP#118).

Both does not crash so I think we can merge this PR until a later refactoring happens in the WS layer.

@inF1704 also successfully tested...

[willmmiles]

This will cause a use-after-free with AsyncTCP 3.5.0. The onDisconnect callback will be run here, so the AsyncClient object will be deallocated; when we return from _handleDataEvent to _onData, the this pointer is invalid; but _onData continues to loop attempting to process more frames. The error state needs to be propagated out so _onData knows to exit early too.

I fixed this in the other branch,
https://github.com/ESP32Async/ESPAsyncWebServer/pull/462/changes#diff-09cab7c8017f6e1b622bbeccebb83bbbd7191e272f10f4330b4f4a4137d875f8L732

[willmmiles]

This name is incorrect: it's used in cases where there is no torn frame. Perhaps WS_SEND_FAILURE?

[willmmiles]

This should return 0 if the add returned 0 -- there's no need to drop the connection if we didn't actually manage to queue anything.

[willmmiles]

This test should be performed prior to line 240.

[willmmiles]

Is there a reason not to simply return the failure code? Packing the value in the status byte only to read it back at the call site is a roundabout solution to a simple problem. Control frames already use that protocol (see line 438), so there's no reason for message frames to differ.

[willmmiles]

Is it OK to set this status here? What happens if we receive frames after we set this but prior to the abort in the next _onPoll()? Do we risk confusing the disconnect state machine if we receive a disconnect request from the client?

[mathieucarbou]

As discussed in #462 with @willmmiles , the current code makes it hard to fix the torn frame issue by aborting without going through more code refactoring.

So I will close this PR in favor of #462 which fixes #453 and refactors the code to be able to abort the WS connection correctly.