Skip to content

lwIP: avoid stale PCB use after remote close - #1679

Open
meganetaaan wants to merge 1 commit into
Moddable-OpenSource:publicfrom
meganetaaan:fix/lwip-tcp-receive-lifetime
Open

lwIP: avoid stale PCB use after remote close#1679
meganetaaan wants to merge 1 commit into
Moddable-OpenSource:publicfrom
meganetaaan:fix/lwip-tcp-receive-lifetime

Conversation

@meganetaaan

Copy link
Copy Markdown
Contributor

Summary

Prevent a delayed receive-window notification from calling tcp_recved() with a freed lwIP tcp_pcb after the remote peer closes a connection while received data remains buffered.

Root cause

When tcpReceive() receives a terminal notification (pb == NULL or a receive error), it currently clears the receive, sent, and error callbacks. Buffered data may still be owned by the socket and consumed later from the XS task.

When that data is consumed, tcp_recved_safe() stores the current raw tcp_pcb * in a heap-allocated message and posts it with tcpip_callback_with_block(). The block argument only blocks until the message can be posted; it does not wait for the callback to run.

The PCB can therefore be destroyed before tcp_recved_INLWIP() executes. Because the terminal receive path already removed tcp_err, tcpError() cannot clear the owner's socket pointer. The queued callback then calls tcp_recved() with a stale PCB.

The observed ESP32-S3 panic ended in:

tcp_update_rcv_ann_wnd
tcp_recved
tcp_recved_INLWIP
tcpip_thread_handle_msg
tcpip_thread

GDB showed that the PCB contents had already been overwritten and that it was no longer present in lwIP's active PCB lists, while the socket owner still retained the old pointer.

Changes

  • On terminal receive, disable the receive and sent callbacks but retain the error callback and callback argument until final teardown.
  • Change tcp_recved_safe() to receive a pointer to the owner's PCB pointer.
  • Use synchronous tcpip_api_call() and re-read that pointer after earlier queued lwIP work has run.
  • Skip tcp_recved() if tcpError() has already invalidated the owner's pointer.
  • Update both the ECMA-419 io/socket implementation and the legacy network/socket implementation.

The synchronous call also removes the heap allocation and deferred callback previously used by tcp_recved_safe().

Relationship to #1655 and #1656

PR #1656 prevents local XS-side socket teardown from racing an lwIP callback that is already being dispatched.

This change addresses a different lifetime direction: after a remote close, an XS-side receive notification can retain a PCB that is destroyed before the queued notification executes.

This PR does not attempt to resolve the separate tcp->buffers node corruption still discussed in #1655.

Validation

Tested on an M5Stack CoreS3 with Moddable 9.0.0 and ESP-IDF 6.0.2.

The reproducer streams data to the device and closes the server side immediately after the response, leaving unread socket buffers. The unpatched build reproduced the tcp_recved() panic and reboot.

With this change:

  • One real-time-paced fixture run terminated normally.
  • Three unpaced, higher-load fixture runs terminated normally.
  • No tcp_recved() panic, task deadlock, or device reboot was observed.
  • The complete CoreS3 application builds successfully.

No automated test is included because the failure depends on the ESP32 lwIP tcpip task and native PCB destruction order; a JavaScript fake socket does not exercise that lifetime.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant