diff --git a/DEVLOG.md b/DEVLOG.md index 8ad36827..fa183aae 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -1,5 +1,53 @@ # Dev Log +## 2026-06-09 — S12.34 drain full lwIP RX pbuf chain + +`LwipRawTcpStream_DrainHeadBytes` read only the head link (`head->len`) then +`pbuf_free(head)` freed the whole chain — silently losing `tot_len - len` tail +bytes whenever lwIP delivered a chained pbuf (segment spanning more than one +pool pbuf). Memory-safe but corrupts the byte stream, and the advertised +TLS-over-LwipRawTcpStream stack carries real TLS record bytes on `_Read`, so the +corruption is network-reachable. Slipped through because the unit test only ever +fabricated single-link pbufs (`len == tot_len`, `next == NULL`). + +### Decisions +- **`tot_len`-keyed drain via `pbuf_copy_partial`.** The cursor (`RxHeadOffset`) + and the fully-drained test now key off `tot_len`, and the copy delegates to + lwIP's own `pbuf_copy_partial`, which walks `head->next`. Chose the platform + primitive over a hand-rolled link walk — one line, idiomatic, less MISRA + surface in Tier-2 code. The issue listed it as an acceptable approach. +- **Cursor/return driven by the actual copied count, not the requested amount.** + Raised by David in review: ignoring `pbuf_copy_partial`'s return is only safe + under lwIP's `tot_len == Σ link->len` invariant. Driving the cursor off the + real return is free and strictly safer — a malformed/overstated `tot_len` + degrades to "delivered fewer bytes / would-block" (StreamSender recovers) + instead of "advance past un-copied bytes and report stale buffer content" + (which would corrupt a stacked TLS stream — the exact bug class). Added a test + pinning it (single link, `len = 2`, `tot_len = 4` → returns 2, not 4). +- **Faithful `pbuf_copy_partial` added to `LwipPbufFake`** (walks `next`, + marshal-guarded like its siblings) so the chained tests exercise a real + multi-link walk rather than a fabricated single link. The TcpStream test exe + links the fake, not upstream `pbuf.c`. +- **Docs:** chained-pbuf contract noted in `docs/integrating-lwip.md`, including + the warning for integrators supplying their own `SolidSyslogStream` byte + transport. + +### Verification +- freertos-host container, debug preset. `SolidSyslogLwipRawTcpStreamTest` + **60 tests green** (3 new: full-chain, cross-boundary partial, overstated + `tot_len`); `SolidSyslogLwipRawDatagramTest` 36 green (shares the fake). pbuf + leak invariant balanced throughout. +- cppcheck-misra (CI invocation): changed file clean. Removing the now-unused + `#include ` shifted three pre-existing cast-helper suppressions — + moved their line numbers in `misra_suppressions.txt` (134/143/151 → + 133/142/150). Local cppcheck 2.10 flags unrelated 5.7/2.4/2.5 in untouched + Core files (version drift vs CI), ignored. +- clang-format clean on all touched code files. + +### Deferred +- The MITM-class no-hostname-verification TLS default is out of scope here — + already tracked/decided in S12.28 (#529). + ## 2026-06-06 — S24.18 drop do/while(0) from test CHECK_* macros Mechanical tree-wide sweep: removed the `do { ... } while (0)` wrapper (and the diff --git a/Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c b/Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c index a42fca41..3b77d4f9 100644 --- a/Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c +++ b/Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c @@ -4,7 +4,6 @@ #include #include #include -#include #include "SolidSyslogError.h" #include "SolidSyslogLwipRawAddressPrivate.h" @@ -388,25 +387,33 @@ static void LwipRawTcpStream_DoRead(void* context) } } -/* Copies up to `size` bytes out of the head pbuf, advancing the read - * cursor. When the head is fully drained, pbuf_free's it and advances - * the queue head — tail entries shift up through the bounded ring via - * modular arithmetic, no compaction. */ +/* Copies up to `size` bytes out of the head pbuf chain, advancing the read + * cursor. The cursor (RxHeadOffset) and the fully-drained test are keyed off + * tot_len, not the first link's len — lwIP hands us a pbuf chain whenever a + * segment spans more than one pool pbuf, and pbuf_copy_partial walks head->next + * for us so the tail links are not lost. The cursor advances by the count + * pbuf_copy_partial actually copied, not the requested amount: under lwIP's + * tot_len == Σ link->len invariant the two are equal, but keying off the real + * copy keeps a malformed/overstated tot_len from advancing past un-copied + * bytes and reporting stale buffer content as received. When the whole chain + * is drained, pbuf_free's it (frees every link) and advances the queue head — + * tail entries shift up through the bounded ring via modular arithmetic, no + * compaction. */ static size_t LwipRawTcpStream_DrainHeadBytes(struct SolidSyslogLwipRawTcpStream* self, void* buffer, size_t size) { struct pbuf* head = self->RxQueue[self->RxQueueHead]; - size_t available = (size_t) head->len - self->RxHeadOffset; + size_t available = (size_t) head->tot_len - self->RxHeadOffset; size_t toCopy = (size < available) ? size : available; - (void) memcpy(buffer, &((const uint8_t*) head->payload)[self->RxHeadOffset], toCopy); - self->RxHeadOffset += toCopy; - if (self->RxHeadOffset >= (size_t) head->len) + size_t copied = (size_t) pbuf_copy_partial(head, buffer, (u16_t) toCopy, (u16_t) self->RxHeadOffset); + self->RxHeadOffset += copied; + if (self->RxHeadOffset >= (size_t) head->tot_len) { (void) pbuf_free(head); self->RxQueueHead = (self->RxQueueHead + 1U) % SOLIDSYSLOG_LWIP_RAW_TCP_RX_QUEUE_SIZE; self->RxQueueCount--; self->RxHeadOffset = 0; } - return toCopy; + return copied; } static void LwipRawTcpStream_EnqueueRxPbuf(struct SolidSyslogLwipRawTcpStream* self, struct pbuf* p) diff --git a/Tests/Lwip/SolidSyslogLwipRawTcpStreamTest.cpp b/Tests/Lwip/SolidSyslogLwipRawTcpStreamTest.cpp index ae16b6c4..08a77b3d 100644 --- a/Tests/Lwip/SolidSyslogLwipRawTcpStreamTest.cpp +++ b/Tests/Lwip/SolidSyslogLwipRawTcpStreamTest.cpp @@ -174,6 +174,39 @@ TEST_BASE(LwipRawTcpStreamTestBase) return result; } + /* Drive the wrapper's tcp_recv callback with a fabricated TWO-link pbuf + * chain (head -> tail), as lwIP delivers whenever a segment spans more + * than one pool pbuf (tot_len > len). The head's tot_len spans both + * links so the wrapper must walk head->next to recover every byte. + * Same ownership / leak-balance contract as pushIncomingPbuf. */ + static err_t pushIncomingChain( + struct pbuf* head, + const void* headData, + uint16_t headLen, + struct pbuf* tail, + const void* tailData, + uint16_t tailLen + ) + { + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) -- pbuf->payload is void*; tests pass string literals + tail->payload = const_cast(tailData); + tail->len = tailLen; + tail->tot_len = tailLen; + tail->next = nullptr; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) -- pbuf->payload is void*; tests pass string literals + head->payload = const_cast(headData); + head->len = headLen; + head->tot_len = static_cast(headLen + tailLen); + head->next = tail; + tcp_recv_fn recvCb = LwipTcpFake_LastRecvFn(); + err_t result = recvCb(LwipTcpFake_LastCallbackArg(), LwipTcpFake_LastTcpNewReturned(), head, ERR_OK); + if (result == ERR_OK) + { + LwipPbufFake_NoteIncomingPbuf(); + } + return result; + } + /* Drive the wrapper's tcp_recv callback with NULL p — peer half-close * (FIN). lwIP retains the pcb; only the receive half is gone. */ static void pushPeerFin() @@ -637,6 +670,64 @@ TEST(SolidSyslogLwipRawTcpStreamConnected, ReadAdvancesPastDrainedPbufToNextInQu LONGS_EQUAL(2, LwipPbufFake_PbufFreeCallCount()); } +TEST(SolidSyslogLwipRawTcpStreamConnected, ReadDrainsEveryLinkOfAChainedPbuf) +{ + struct pbuf link1 = {}; + struct pbuf link2 = {}; + pushIncomingChain(&link1, "AB", 2, &link2, "CD", 2); + + SolidSyslogSsize n = readBytes(); + + LONGS_EQUAL(4, n); + MEMCMP_EQUAL("ABCD", readBuffer, 4); +} + +TEST(SolidSyslogLwipRawTcpStreamConnected, ReadAcrossChainedPbufLinkBoundaryPreservesByteOrder) +{ + struct pbuf link1 = {}; + struct pbuf link2 = {}; + pushIncomingChain(&link1, "AB", 2, &link2, "CD", 2); + + /* A 3-byte read straddles the link boundary — two bytes from link1, one + * from link2 — and the chain is not yet fully drained. */ + SolidSyslogSsize first = readBytes(3); + + LONGS_EQUAL(3, first); + MEMCMP_EQUAL("ABC", readBuffer, 3); + CALLED_FAKE(LwipPbufFake_PbufFree, NEVER); + + /* The final byte comes from the tail link; the whole chain frees once. */ + SolidSyslogSsize second = readBytes(); + + LONGS_EQUAL(1, second); + MEMCMP_EQUAL("D", readBuffer, 1); + CALLED_FAKE(LwipPbufFake_PbufFree, ONCE); +} + +TEST(SolidSyslogLwipRawTcpStreamConnected, ReadReportsOnlyBytesActuallyCopiedWhenTotLenOverstatesChain) +{ + /* A malformed pbuf: tot_len claims 4 bytes but the single link holds only + * 2 and there is no next link. The drain must report what lwIP actually + * copied (2), never the phantom tail the overstated tot_len implies — + * advancing past un-copied bytes would feed stale buffer content to a + * stacked TLS record stream. */ + struct pbuf link = {}; + const char data[] = "AB"; + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) -- pbuf->payload is void*; tests pass string literals + link.payload = const_cast(static_cast(data)); + link.len = 2; + link.tot_len = 4; + link.next = nullptr; + tcp_recv_fn recvCb = LwipTcpFake_LastRecvFn(); + (void) recvCb(LwipTcpFake_LastCallbackArg(), LwipTcpFake_LastTcpNewReturned(), &link, ERR_OK); + LwipPbufFake_NoteIncomingPbuf(); + + SolidSyslogSsize n = readBytes(); + + LONGS_EQUAL(2, n); + MEMCMP_EQUAL("AB", readBuffer, 2); +} + TEST(SolidSyslogLwipRawTcpStreamConnected, SendReturnsFalseAfterPeerFin) { pushPeerFin(); diff --git a/Tests/Support/LwipFakes/Source/LwipPbufFake.c b/Tests/Support/LwipFakes/Source/LwipPbufFake.c index df4c781a..e439ebf4 100644 --- a/Tests/Support/LwipFakes/Source/LwipPbufFake.c +++ b/Tests/Support/LwipFakes/Source/LwipPbufFake.c @@ -1,6 +1,7 @@ #include "LwipPbufFake.h" #include +#include #include "LwipFakeMarshalGuard.h" #include "lwip/arch.h" @@ -105,3 +106,34 @@ u8_t pbuf_free(struct pbuf* p) --outstandingPbufCount; return 1; } + +/* Faithful model of lwIP's pbuf_copy_partial: copy up to `len` bytes out of + * the pbuf chain `p`, starting `offset` bytes in, walking `p->next` across + * links. Returns the number of bytes copied. Lets the chained-pbuf tests + * exercise the wrapper's tot_len-keyed drain just as real lwIP would. */ +u16_t pbuf_copy_partial(const struct pbuf* p, void* dataptr, u16_t len, u16_t offset) +{ + LWIP_REQUIRE_MARSHAL_ACTIVE(); + uint8_t* out = (uint8_t*) dataptr; + u16_t skip = offset; + u16_t copied = 0; + const struct pbuf* link = p; + while ((link != NULL) && (copied < len)) + { + if (skip >= link->len) + { + skip = (u16_t) (skip - link->len); + } + else + { + u16_t linkAvailable = (u16_t) (link->len - skip); + u16_t want = (u16_t) (len - copied); + u16_t take = (want < linkAvailable) ? want : linkAvailable; + (void) memcpy(&out[copied], &((const uint8_t*) link->payload)[skip], take); + copied = (u16_t) (copied + take); + skip = 0; + } + link = link->next; + } + return copied; +} diff --git a/docs/integrating-lwip.md b/docs/integrating-lwip.md index f898d3ea..209c0c37 100644 --- a/docs/integrating-lwip.md +++ b/docs/integrating-lwip.md @@ -353,6 +353,18 @@ calls `tcp_recved(pcb, n)` to ACK back to lwIP's receive window, and callback returns non-`ERR_OK` so lwIP retains the pbuf and replays the callback later (lwIP's flow-control hook). +> **Chained pbufs.** lwIP hands a single received segment as a **pbuf +> chain** (`p->tot_len > p->len`, `p->next != NULL`) whenever the +> payload spans more than one pool pbuf — normal once a segment exceeds +> one `PBUF_POOL_BUFSIZE`, and influenced by the peer / on-path +> segmentation. The wrapper drains the *whole chain* keyed off +> `tot_len` (via `pbuf_copy_partial`, walking `p->next`) across one or +> more `Stream_Read` calls, and `pbuf_free`s the head — which frees +> every link — only once the chain is fully consumed. Integrators +> supplying their own `SolidSyslogStream` byte transport must honour the +> same contract: never read only `head->len` and then free the chain, or +> the tail bytes are lost (this corrupts a stacked TLS record stream). + The default-8 queue size is sized for the typical mTLS handshake flight (ServerHello + Certificate + ServerKeyExchange + ServerHelloDone is 2–4 segments). Bump it for streaming server diff --git a/misra_suppressions.txt b/misra_suppressions.txt index 2478a9ce..6235cf32 100644 --- a/misra_suppressions.txt +++ b/misra_suppressions.txt @@ -44,7 +44,7 @@ misra-c2012-11.3:Platform/LwipRaw/Source/SolidSyslogLwipRawAddressStatic.c:35 misra-c2012-11.3:Platform/LwipRaw/Source/SolidSyslogLwipRawAddressStatic.c:55 misra-c2012-11.3:Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c:65 misra-c2012-11.3:Platform/LwipRaw/Source/SolidSyslogLwipRawDnsResolver.c:206 -misra-c2012-11.3:Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c:134 +misra-c2012-11.3:Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c:133 misra-c2012-11.3:Platform/PlusTcp/Source/SolidSyslogPlusTcpAddress.c:14 misra-c2012-11.3:Platform/PlusTcp/Source/SolidSyslogPlusTcpAddressPrivate.h:24 misra-c2012-11.3:Platform/PlusTcp/Source/SolidSyslogPlusTcpAddressPrivate.h:31 @@ -84,8 +84,8 @@ misra-c2012-11.5:Core/Source/SolidSyslogUdpSender.c:227 misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawDatagram.c:74 misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawDnsResolver.c:149 misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawDnsResolver.c:211 -misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c:143 -misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c:151 +misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c:142 +misra-c2012-11.5:Platform/LwipRaw/Source/SolidSyslogLwipRawTcpStream.c:150 misra-c2012-11.5:Platform/MbedTls/Source/SolidSyslogMbedTlsStream.c:298 misra-c2012-11.5:Platform/MbedTls/Source/SolidSyslogMbedTlsStream.c:310 misra-c2012-11.5:Platform/Windows/Source/SolidSyslogWinsockDatagram.c:142