Guard server receive window against rollover and bound debug client copy#11
Open
sclaiborne wants to merge 1 commit into
Open
Guard server receive window against rollover and bound debug client copy#11sclaiborne wants to merge 1 commit into
sclaiborne wants to merge 1 commit into
Conversation
The server-side HttpShiftReceivePointer decremented MaxReceiveLength without a rollover check, so an accumulated request larger than the receive buffer wrapped the UDINT and let the TCP layer overrun the buffer. Mirror the client-side guard: refuse the shift, raise LLHTTP_ERR_PACKET_SIZE_TOO_BIG, and drop the connection. The end-of-cycle debug copy read sizeof(internal.clients) from pClients, which is allocated with only numClients elements, reading past the allocation whenever numClients < LLHTTP_MAX_NUM_CLIENTS. Bound the copy by both sizes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #10 (based on
fix/server-request-body); will retarget tomainonce that merges.Fixes two robustness bugs in
src/Ar/LLHttp/HttpServer.c:Receive window rollover (buffer overflow)
HttpShiftReceivePointer()decrementedMaxReceiveLengthwith no rollover check (the old// TODO: Dont allow rollover). If an accumulated partial request exceeded the receive buffer,MaxReceiveLengthwrapped (UDINT) and the TCP layer could overrun the buffer. The function now mirrors the client-sidehttpShiftReceivePointer()guard: it refuses the shift, and the call site raisesLLHTTP_ERR_PACKET_SIZE_TOO_BIGand drops the connection — same policy as the client, which closes the socket on this error.Heap overread in debug client copy
The end-of-cycle debug copy read
sizeof(internal.clients)(5 elements) frompClients, which is TMP_alloc'd with onlynumClientselements — reading past the allocation every cycle whenevernumClients < LLHTTP_MAX_NUM_CLIENTS. The copy is now bounded bymin(numClients, LLHTTP_MAX_NUM_CLIENTS)elements.Both changed functions are file-local, so no
.fun/.typchanges.Tests
Two new tests in
test/tests.cpp(run off-PLC via the loupeRT harness, 32-bit MinGW):content-lengthinto a server with a 2000-byte buffer in segments (each capped at the advertisedMaxReceiveLength, like the real TCP layer) until the buffer is exhausted; asserts the server errors withLLHTTP_ERR_PACKET_SIZE_TOO_BIG, drops the connection, and the receive window never extends past the allocated buffer.numClients = 2, plants a sentinel ininternal.clients[2..4], and asserts a cycle mirrors the first 2 elements while leaving the sentinel untouched.Verified 9/9 tests pass with the fix; reverting only
HttpServer.cto the parent commit makes exactly the two new tests fail.🤖 Generated with Claude Code