[willmmiles]

Apologies in advance for the monster PR. The development work from #123 identified a substantial number of use after frees, resource leaks, and other unspecified behavior issues. This PR checkpoints core changes to resolve most of the issues discovered so far.

Fixed issues broken down by category:

Opening a connection

No way to cancel an outstanding connection attempt (DNS or otherwise)

client->connect("slow.example", 8080);
client->close();     // or abort()

Naiively, one might expect that this would cancel the pending connection. Instead the close() is ignored, and the connection will proceed.

A failed connect() leaks the pcb and its local port

if (!client->connect(host, 8080)) {
   // pcb was leaked!
}

As described - connect() returns false with no cleanup. One pcb and one local port are lost per failed dial, which is the common case when WiFi drops and the application retries.

setNoDelay() before the handshake completes is a silent no-op

client->connect(host, 8080);
client->setNoDelay(true);   // ignored; getNoDelay() returns false

Both accessors early-return while the pcb is still unattached. (This follows from the late pcb adoption noted at the end rather than being an independent bug, but the effect on the application is real.)

Connecting to a host by name

A late DNS answer dereferences a destroyed client

client->connect("slow.example", 443);
delete client;   // or it goes out of scope

lwIP cannot cancel an outstanding lookup, and the callback argument was the raw AsyncClient *. When the answer arrives, the library reads the destroyed object and attempts to connect, resulting in a use-after-free.

A failed lookup connects to 0.0.0.0 instead of reporting the failure

A name that does not resolve produces no onError/onDisconnect at all. The library zeroes the address when the resolver reports failure and then takes the success path, so it allocates a pcb, dials 0.0.0.0, and leaks the pcb. Also possible on the immediate-connect path if NXDOMAIN was cached.

A stale lookup answer connects to the host you moved on from

Lookups carry no identity, so a superseded one cannot recognise itself. Calling connect() a second time while the first lookup is outstanding leaves you connected to the first host (if the stale answer arrives first), or with the second connect() silently producing no connection at all. Both a close()-then-reconnect and a straight second connect() hit this.

Handling errors and reconnecting

Reconnecting from inside onError misreports the connection

client->onError([](void *, AsyncClient *c, int8_t) { c->connect(host, port); });

onDisconnect fires immediately afterwards, so the application is told the replacement connection dropped. Reconnecting by name behaves differently again: the reset arrives as onDisconnect rather than onError, and the reconnect attempted from the handler never happens.

connect() succeeds while an error event is still undelivered

A reset that has been observed but not yet dispatched leaves the client looking idle, so a fresh connect() is accepted — and the pending reset is then reported against the connection that replaced it. A client that still owes a notification should refuse to connect.

Destroying or closing a client

close() inside onData drops the acknowledgement

client->onData([](void *, AsyncClient *c, void *, size_t) { /* handle */ c->close(); });

The ack is emitted only if a pcb is still attached, and close() has already detached it. The peer is sent a TCP RST for data that was in fact delivered.

This was masked by the _rx_ack_len member being uninitialized, so we would often tcp_recved an absurd number of bytes before processing the close, resulting in some connections apparently closing correctly.

A withheld ack carries over into the next connection

After ackLater() on one connection, closing and reconnecting charges the old debt against
the new pcb: the first ack() on the new connection acknowledges bytes that arrived on the
previous one. Per-connection state is never reset when a pcb is adopted.

Deleting the client inside onData crashes

client->onDisconnect([](void *, AsyncClient *c) { delete c; });
client->onData([](void *, AsyncClient *c, void *, size_t) { if (transaction_complete) c->close(); });

The receive loop continues after the handler returns and reads the freed object. This is most likely to happen as a result of client->close() being called in the handler when the transaction completes, resulting in the onDisconnect() handler deleting the client object.

Deleting the client inside onError crashes

client->onError([](void *, AsyncClient *c, int8_t) { delete c; });

The error path runs onDisconnect unconditionally against the same object straight afterwards. Results in a use-after-free.

Destroying a client after a reset crashes and swallows onDisconnect

If the peer resets and the application destroys the client before the event is dispatched, the destructor does nothing — it only cleans up when a pcb is still attached, and the reset already cleared it. The queued event later dispatches against freed memory, and the owed onDisconnect is never delivered. Use-after-free plus a missing callback.

AsyncServer

Destroying an AsyncServer with an accept in flight crashes

A queued accept holds a raw pointer to the server, so a connection accepted but not yet delivered dereferences the server after it has gone out of scope, resulting in a use-after-free.

A failed listen() leaks the bound pcb and holds the port

Bind and listen are not unwound as one transaction: when the listen pcb cannot be allocated, the bound pcb is never freed and the port is held for the life of the process, so no later begin() on that port can ever succeed.

A connection reset before delivery leaks the client object

If the peer resets between accept and delivery, the queued event is purged but the AsyncClient it owned is not. 360 bytes leaked per occurrence.

end() still delivers connections from the listener it closed

server.end();   // a connection was accepted but not yet delivered

onClient is invoked anyway, handing the application a connection on a listener it has already shut down. The pcb and client are leaked.

Restarting a server resurrects an accept the previous session disowned

end() followed by begin() makes the stale queued accept deliverable again, so a connection belonging to the closed listening session is handed to the new one. Same leak.

Other fixes and improvements

AsyncTCPSimpleIntrusiveList uninitialized member

The list element count was not initialized. This only "worked" because the one use case (the main queue) was instantiated in zero'd memory at program startup.

Fully compile out callback timing if not being collected

Unfortunately it turns out the optimizer can't resolve micros(), so it was building in these calls even when the logs themselves were eliminated. Update the build guard to avoid the unnecessary work unless the logs are being generated.

Measure available write() size atomicly

Ensure we can write the most bytes by doing the check in the LwIP lock scope instead of before.

Improve core locking technique

Add a metafunction allowing code blocks requiring LwIP core mutexing to be inlined in to their calling functions, keeping the relevant code together. This is also slightly faster when mutex-based core locking is available or unnecessary (arduino-3, ESP8266 or other non-RTOS platforms) and falls back cleanly to tcpip_call_api when not.

---

Alongside this effort, Claude and I began developing a formal test suite; I will PR that separately as it's still in a bit of a nascent form (and still turning up more issues).

[willmmiles]

LibreTiny CI issues seem to a result of an incompatibility with PlatformIO 6.2.0.

[Copilot]

Copilot review overview

🟡 Changes recommended

Queued callbacks can survive teardown, accept-state checks race across tasks, and allocation-failure paths can crash.

Review effort: Balanced
Findings: 2 · 2

Open (4)

• Synchronize accept-validity state across tasks · New
• Handle shared allocation failure in the LwIP accept callback · New
• Guard destructor queue mutex use after initialization failure · New
• Purge queued client events after detaching the PCB · New

What changed in this PR

Refactors client/server lifetimes and lwIP operations to prevent use-after-free, leaks, stale callbacks, and connection-state corruption.

Changes:

• Introduces shared implementation objects for safe asynchronous lifetimes.
• Makes DNS, connection teardown, and server acceptance transactional.
• Improves locking, acknowledgement handling, assertions, and queue initialization.

File	Description
src/​AsyncTCP.cpp	Implements lifecycle, locking, DNS, teardown, and server fixes.
src/​AsyncTCP.h	Adds implementation facades and ownership declarations.
src/​AsyncTCPSimpleIntrusiveList.h	Initializes the list size.
Kconfig.projbuild	Adds optional runtime assertions.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mathieucarbou]

Here is an AI summary of the improvements for readers:

Scenario	Main	PR
Delete client in onData	💥 Crash (UAF — _recv loop reads freed object)	✅ Safe (impl survives via shared_ptr, facade null-checked)
Delete client in onError	💥 Crash (UAF — _error fires onDisconnect on freed object)	✅ Safe (_error holds shared_from_this(), checks _facade)
Delete client after RST, before dispatch	💥 Crash + missing onDisconnect	✅ Safe (destructor unconditionally closes, queued event drains, onDisconnect fires)
close() in onData	⚠️ RST instead of FIN (ack dropped)	✅ Correct FIN (_in_callback_ack_len acked in close transaction)
Failed DNS lookup	⚠️ Connects to 0.0.0.0, leaks pcb, no callback	✅ onError(-55) + onDisconnect, no leak
Stale DNS answer	⚠️ Connects to wrong host	✅ Dropped (token mismatch)
connect() while error pending	⚠️ Accepted, error misattributed to new connection	✅ Refused (_dispose_owed check)
Reconnect from onError	⚠️ onDisconnect fires for the new connection	✅ Suppressed (_dispose_owed claimed before onError)
Destroy server with accept in flight	💥 Crash (UAF on AsyncServer*)	✅ Safe (event holds shared_ptr<AsyncServerImpl>, facade null-checked at dequeue)
end() then pending accept	⚠️ Delivered to closed listener, pcb+client leaked	✅ Dropped at dequeue (epoch mismatch)
end() + begin() with pending accept	⚠️ Stale accept delivered to new session, leaked	✅ Dropped (epoch mismatch)
Failed listen()	⚠️ Bound pcb leaked, port held forever	✅ pcb closed in same transaction
Reset between accept and delivery	⚠️ 360-byte AsyncClient leaked	✅ No facade allocated until delivery; impl closed on discard
ackLater() then reconnect	⚠️ Old ack debt charged to new pcb	✅ _adopt() resets _rx_ack_len
setKeepAlive(0, ...) on null pcb	💥 Crash (null deref)	✅ Returns ERR_CONN (guarded in lock lambda)
setNoDelay()/setKeepAlive() concurrent with LwIP	⚠️ Data race on pcb fields	✅ Serialized via with_tcp_core_lock
write() partial-write sizing	⚠️ TOCTOU between space() check and tcp_write()	✅ Sized inside the core lock
free() during DNS lookup	⚠️ Returns true (looks idle)	✅ Returns false (still connecting)
_sent for stale pcb	⚠️ No guard — fires onAck with wrong pcb	✅ Guarded (_pcb != pcb → ERR_CONN)
_fin for stale pcb	⚠️ Calls close() unconditionally	✅ Guarded (_pcb != pcb → ERR_CONN)
IPv6 connection getRemoteAddress()	⚠️ Returns garbage (reads .ip4 from v6 addr)	✅ Type-checked, returns 0 for mismatched type
tcp_poll for SYN_SENT connection	⚠️ Queues poll events uselessly	✅ Early return in LwIP context
pbuf tot_len after dechain	⚠️ Incorrect (stale chain total)	✅ Fixed (b->tot_len = b->len)
SimpleIntrusiveList::_size	⚠️ Uninitialized (worked by luck of zeroed BSS)	✅ Initialized to 0

[Copilot]

🟡 Changes recommended

Connection-state races, facade lifetime risks, stale acknowledgements, and unresolved pre-DNS setNoDelay() behavior remain.

5 open findings

• Protect facade lifetime during event dispatch · New
• Use connection generations instead of PCB pointer equality · New
• Revalidate connection state before allocating a replacement PCB · New
• Recheck pending terminal events before starting hostname resolution · New
• Persist and apply NoDelay setting after asynchronous DNS connection · New

4 resolved since last review

• Handle shared allocation failure in the LwIP accept callback
• Synchronize accept-validity state across tasks
• Purge queued client events after detaching the PCB
• Guard destructor queue mutex use after initialization failure

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[willmmiles]

This is technically correct, however I believe that the likelihood of the case is not worth the memory cost of having to carry around a generation counter in every object. I'll leave a comment to that effect, and I'll review it in the context of the OOM handling - there might be some synergy there.

[willmmiles]

Another one for with_tcp_core_lock(), I think... I'd held off on doing the full conversion, but perhaps it's necessary.

[willmmiles]

No. We'd have to increase the size of the AsyncClientImpl to carry around one extra bool all the time; it's not worth the memory cost. This is not a new behaviour in the PR - main has also never persisted the value prior to the connection callback. It's the example that's wrong.

[mathieucarbou]

@willmmiles for the std::make_shared that is throwing... maybe this non throwing alternative ?

diff --git a/src/AsyncTCP.cpp b/src/AsyncTCP.cpp
index e37f7b3..00b0336 100644
--- a/src/AsyncTCP.cpp
+++ b/src/AsyncTCP.cpp
@@ -167,6 +167,37 @@ template<typename T> err_t with_tcp_core_lock(T &&func) {
 #endif
 }
 
+/*
+  Soft-fail shared_ptr construction.
+
+  std::shared_ptr has no noexcept form: constructing one from a raw pointer allocates a
+  control block which may throw (or terminate under -fno-exceptions) on failure.  This
+  helper allocates the object with nothrow first -- the bulk of the allocation -- and
+  only the small control-block allocation remains fallible.  Where exceptions are
+  available a control-block failure is caught and reported as a null shared_ptr.
+  Under -fno-exceptions the control-block allocation can still terminate, but the much
+  larger object allocation is always soft-fail.
+*/
+template<typename T, typename... Args> std::shared_ptr<T> make_shared_nothrow(Args &&...args) {
+  std::unique_ptr<T> guard(new (std::nothrow) T(std::forward<Args>(args)...));
+  if (!guard) {
+    return nullptr;
+  }
+#ifdef __EXCEPTIONS
+  try {
+    std::shared_ptr<T> sp(guard.get());
+    guard.release();
+    return sp;
+  } catch (...) {
+    return nullptr;  // guard frees the object
+  }
+#else
+  std::shared_ptr<T> sp(guard.get());
+  guard.release();
+  return sp;
+#endif
+}
+
 }  // anonymous namespace
 
 /*
@@ -2353,7 +2384,15 @@ int8_t AsyncTCP_detail::tcp_accept(void *arg, tcp_pcb *pcb, int8_t err) {
   // The facade will be constructed by the async task when it processes this event -- if the connection
   // errors out prior to accept running, the event will be discarded and the implementation destroyed
   // as there's no API to inform the server of the error.
-  e->impl = std::make_shared<AsyncClientImpl>(nullptr);
+  // Soft-fail: this runs on the LwIP thread, where a throwing make_shared would terminate
+  // the network stack.  See make_shared_nothrow above.
+  e->impl = make_shared_nothrow<AsyncClientImpl>(nullptr);
+  if (!e->impl) {
+    async_tcp_log_e("_accept failed: couldn't allocate client impl");
+    tcp_abort(pcb);
+    delete e;
+    return ERR_ABRT;
+  }
   e->impl->_adopt(pcb);
   e->server = server->shared_from_this();  // Lock server impl in scope
   e->accept.epoch = server->_epoch;        // And remember in case it's end()ed

still some copilot comments to review and see whether the non throwing version of make_shared is included or not

[willmmiles]

@willmmiles for the std::make_shared that is throwing... maybe this non throwing alternative ?

If only it was that easy! This line can still throw:

std::shared_ptr<T> sp(guard.get());

The problem is that the shared_ptr constructor still needs to allocate the reference count itself. So all that code buys you no guarantees at all.

Also, that particular implementation is pessimal on the allocator, it guarantees always using two separate allocations (one for the object, one for the control block). std::make_shared<> has special privileges on std::shared_ptr<> that allow it to allocate the reference count along with the storage for the object itself in a single transaction.

I do have an actually safe prototype implementation already, but I was holding to back for now as there's half a dozen other "allocation failure will break things" issues as well -- it's not only shared_ptr that will fall over, and this PR is already big enough. If you'd like me to add all of that to this PR, I can put the rest of it together.

[mathieucarbou]

The problem is that the shared_ptr constructor still needs to allocate the reference count itself. So all that code buys you no guarantees at all.

Yes I totally agree. It does not guarantee this part but was only handling the case for the object allocation.
Let's wait for your solution, except if you feel we can merge the PR without it first and add it later.
As you want! 👍

[mathieucarbou]

Hi @willmmiles : what is the status of your PR ? ready to be reviewed / merged ?