Repository navigation
httpserver: Size the request body from Content-Length (H7) - #111
Conversation
Three concurrent PATCH requests with ~6kB bodies reboot the device within
seconds. The serial log names it exactly:
[system] realloc(0, 5504) failed, resetting
The device had 31kB free at that moment and still could not obtain 5504
bytes, so this was fragmentation rather than exhaustion.
HTTP_STATE_BODY grew the body buffer one received byte at a time through
realloc_chunks(), which meant ~94 reallocs for a 6kB body, each abandoning
a block exactly REALLOC_CHUNK_SIZE smaller than the next request. With
several bodies growing in lockstep those blocks sit between each other's
live buffers and cannot coalesce, so the free list fills with a ladder of
blocks that are each just too small. 5504 is 86 x 64, a realloc_chunks()
step.
alloc_body() now sizes the buffer from Content-Length in a single
allocation, so the ladder never forms. It refuses the body with a 400 when
the length is out of range or when granting it would leave less than
HTTP_BODY_MIN_FREE_MEM free; checking before allocating is what turns "no
memory" into a response rather than a reset, since my_malloc() reboots on
failure. An over-long Content-Length is now rejected before any buffering,
where previously the device accepted 10240 bytes first and only then gave
up.
realloc_chunks() doubles up to REALLOC_MAX_CHUNK_SIZE instead of adding a
fixed step, for the callers that genuinely cannot know their final size
(JSON dumps), at the cost of at most 1kB of tail.
Note this is not a regression from the 1.3.0 refactor: version-1.2.9 fails
the same way on the same test (realloc(0, 5144) failed, resetting).
Verified on the TestMyESP bench: the test that reboots both 1.2.9 and
1.3.0-beta.1 passes with no allocation failures and free heap holding at
~30kB, and the full API suite runs with zero resets.
There was a problem hiding this comment.
🔵 Needs a closer look
The change reworks a memory-critical parser state machine on a reboot-on-OOM allocator where fragmentation behavior can only be validated on real hardware, warranting final human review.
1 open issue
What changed in this PR
This PR fixes the H7 memory-fragmentation finding in the HTTP server. Previously HTTP_STATE_BODY grew the request body buffer one received byte at a time via realloc_chunks(), which on the ESP8266's fragmentation-prone allocator produced a ladder of orphaned, just-too-small blocks when several large bodies arrived concurrently — causing my_realloc() to fail and reboot the device even with plenty of free heap. The change sizes the body buffer in a single Content-Length-driven allocation and refuses oversized/low-memory requests with a 400 before allocating (necessary because the allocator reboots on failure rather than returning NULL). It fits into the firmware's broader memory-safety discipline documented for the HTTP front end.
Changes:
alloc_body()allocates the body buffer once fromContent-Length, validates the length range, and refuses the request (400) when granting it would drop free heap belowHTTP_BODY_MIN_FREE_MEM.- Simplifies the parser state machine:
HTTP_STATE_HEADER_READYis now only the no-body terminal state, andHTTP_STATE_BODYno longer reallocates per byte. realloc_chunks()now grows by doubling (up toREALLOC_MAX_CHUNK_SIZE) instead of a fixed step, reducing reallocation churn for size-unknown callers (JSON dumps).
| File | Description |
|---|---|
| src/espgoodies/utils.c | Replaces fixed-step growth in realloc_chunks() with capped doubling; adds REALLOC_MAX_CHUNK_SIZE. |
| src/espgoodies/httpserver.h | Adds HTTP_BODY_MIN_FREE_MEM guard constant with rationale. |
| src/espgoodies/httpserver.c | Adds alloc_body(), includes user_interface.h, and reworks the header/body parser states to allocate once and reject out-of-range/low-memory bodies with 400. |
Review effort: Balanced
| free(hc->body); | ||
| hc->body = NULL; | ||
| hc->body_len = 0; | ||
| hc->body_alloc_len = realloc_chunks(&hc->body, 0, hc->content_length + 1); |
There was a problem hiding this comment.
Fixed in f9c6e30: alloc_body() now allocates content_length + 1 directly instead of going through realloc_chunks(). You are right that the doubling is meant for callers that cannot know their size — here it cost up to 1023 bytes per body (10240 → 11264, exactly as you noted), which is up to ~4kB across the four parallel contexts on a heap this PR is trying to conserve. body_alloc_len is still set, so the HTTP_STATE_BODY bounds check is unaffected; I checked the terminator still lands on the last valid index for every length from 1 to HTTP_MAX_BODY_LEN.
alloc_body() knows the final size, so going through realloc_chunks() only made it inherit that function's doubling growth. A 10240 byte body took an 11264 byte block, and the worst case wastes 1023 bytes per body, up to about 4kB across the four parallel contexts. The doubling is there for callers that cannot know their final size, such as the JSON dumps, and realloc_chunks() keeps it for them. body_alloc_len is still set, so the HTTP_STATE_BODY bounds check is unaffected: the terminator lands on the last valid index for every length from 1 to HTTP_MAX_BODY_LEN.
Fixes the
H7finding inCODE_REVIEW.md, reproduced and verified on the TestMyESP bench.The failure
Three concurrent
PATCHrequests with ~6kB bodies, sent while a/listenresponse was being built, reboot the device within seconds:The device had 31kB free at that moment and still could not obtain 5504 bytes. This was never exhaustion — it was fragmentation.
Cause
HTTP_STATE_BODYgrew the body buffer one received byte at a time throughrealloc_chunks(). A 6kB body meant ~94 reallocs, each abandoning a block exactlyREALLOC_CHUNK_SIZEsmaller than the next request. With several bodies growing in lockstep, those abandoned blocks sit between each other's live buffers and cannot coalesce, so the free list fills with a ladder of blocks that are each just too small, however much of it there is.5504 is 86 × 64, i.e. a
realloc_chunks()step — that is what pinned it down.Changes
alloc_body()sizes the body buffer fromContent-Lengthin a single allocation, so the ladder never forms.HTTP_BODY_MIN_FREE_MEMfree. Checking before allocating is the only way to turn "no memory" into a response rather than a reset, sincemy_malloc()reboots on failure.Content-Lengthis now rejected before any buffering. Previously the device accepted 10240 bytes and only then gave up.realloc_chunks()doubles up toREALLOC_MAX_CHUNK_SIZEinstead of adding a fixed step, for callers that genuinely cannot know their final size (JSON dumps), at the cost of at most 1kB of tail.Not a 1.3.0 regression
version-1.2.9fails the same way on the same test:This is reachable in every release with that growth pattern.
Verification
config_name)DEBUG=falsebuild (adds-Werror)Body-carrying test groups all pass:
put-device-attributes,set-device-attributes*,set-peripherals, the nineset-port-sequence*cases, and the twelvewebhooks-*cases.