Reduce undefined behavior

[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.