fix(Outbox): re-anchor it._prev after _remove() to prevent dangling pointer

[dj803]

Problem

remove(Iterator& it) calls ++it to advance the iterator before passing the predecessor pointer prev to _remove(). The ++it operator sets it._prev = node (the node that _remove is about to free). After _remove(prev, node) deallocates node, it._prev holds a dangling pointer.

A second call to remove(it) extracts prev = it._prev (now dangling) and passes it to _remove(). Depending on which internal branch fires:

_last = prev;            // writes to freed memory
_last->next = nullptr;   // dereferences freed memory
prev->next = node->next; // dereferences freed memory

This corrupts whatever heap block was recycled into that slot and produces use-after-free crashes in the caller. The failure surface is any code path that calls _clearQueue(0) on TCP disconnect while outbox items remain — exactly what happens on an unexpected TCP RST.

We observed five distinct crash signatures on an ESP32 fleet (three separate devices, confirmed single root cause): LoadProhibited faulting on Packet::packetType / _data[0], a heap_caps assertion in ~Packet freeing _data, two lwIP tcp_input assertions, and a lwip_netconn_do_close_internal LoadProhibited — all traceable to a single corrupted outbox node after a TCP RST disconnection.

Fix

Capture all iterator state before the removal, then rebuild both fields explicitly — avoiding ++it entirely (suggested by @bertmelis):

  void remove(Iterator& it) {  // NOLINT(runtime/references)
    if (!it) return;
    Node* node = it._node;
    Node* prev = it._prev;
    Node* next = node->next;
    _remove(prev, node);
    it._prev = prev;
    it._node = next;
  }

prev is the live predecessor that _remove just used to relink the queue — it remains valid. next is captured before the removal so it is never read through the freed node.

Test

Added test_outbox_remove_consecutive (improved by @bertmelis) which calls remove(it) twice in succession from the middle of a five-element outbox. Without the fix, the second call passes a dangling pointer to _remove(); with the fix the size is correct and no memory is corrupted.

The test suite is also run without the memory pool (EMC_USE_MEMPOOL=0) so Valgrind can detect use-after-free — with the pool enabled, recycled memory masks the UB.

Validation

Validated on an 8-device ESP32 fleet: 9-hour soak after applying this patch, including 4 absorbed TCP RST events on one device during an extended 14-hour run, 0 post-patch coredumps across the full fleet.

[bertmelis]

I added your test without the fix and it passed. I want to reproduce using a test before merging.

Are you using the async version and would it be possible to share a (decoded) stack trace??

Don't get me wrong, the current code partly invalidates the iterator and that is a bug. I'm just trying to figure out a scenario to test.

[bertmelis]

I did some testing.

• I didn't test for consecutive removals
• Valgrind doesn't complain because mempool is enabled so memory is cleaned up and UB isn't detected

So I created a branch with the testing issues fixed. I also improved your solution. Feel free to cherry-pick and adjust your PR.

  // remove node at iterator, iterator points to next
  void remove(Iterator& it) {  // NOLINT(runtime/references)
    if (!it) return;
    // capture iterator state
    Node* node = it._node;
    Node* prev = it._prev;
    Node* next = node->next;

    // remove element
    _remove(prev, node);

    // rebuild iterator state
    it._prev = prev;
    it._node = next;
  }

[dj803]

Yes, AsyncTCP-backed — bertmelis/espMqttClient @ 1.7.2 with ESP32Async/AsyncTCP ≥ 3.4.0, on ESP32 / arduino-esp32 3.2.0 / ESP-IDF 5.3.2.

I do have decoded stack traces from the five crashes but they're not going to be very useful here — they're crash-site backtraces (downstream heap corruption victims: Packet::packetType() LoadProhibited, ~Packet free(_data) heap_caps assert, lwIP tcp_input/netconn assertions), not the corruption site itself. The Outbox::remove call chain doesn't appear in any of them, and the raw addresses are from our application binary so they won't decode without our ELF.

What might be more useful:

Trigger: _clearQueue(0) on TCP RST with ≥ 2 items in the outbox. The library calls remove(it) in a loop to drain the queue; the second call is the one that reads through the dangling it._prev.

Validation: After patching, we absorbed 4 TCP RST events on one device over a 14-hour continuous run with zero coredumps. Unpatched, any of those RST events would have crashed within the next _clearQueue drain cycle.

Also — I'll incorporate your improved remove() implementation from test_dangling-pointer. Capturing all three fields before the removal and avoiding the ++it altogether is much cleaner than my two-step approach.

[dj803]

Done — updated the PR to use your implementation from test_dangling-pointer. Three cherry-picked commits (mempool-off test, consecutive-removal test, your improved remove()) are now the tip of this branch.

The current state:

• fix dangling pointer commit carries your exact code (capture node/prev/next up-front, call _remove, then rebuild both iterator fields)
• test remove consecutive adds the multi-removal regression test with and without mempool
• PR description updated to credit you and use your formulation

Let me know if anything needs adjusting before merge.

[bertmelis]

Everything is fine! Thank you for the bugfix! According to my local AI there is no clear path that reveals the issue because it is UB. It probably manifests under high load or async operations only.

[dj803]

My pleasure. I'm glad i could help. :)