Repository navigation
Fix request processing failures caused by failed allocations - #501
mathieucarbou wants to merge 8 commits into
Conversation
|
@willmmiles : this is a starting point, not fully tested yet. |
|
Apart from If we want to support handling allocation failure on platforms with |
That’s true, but maybe we can do it in 2 pass ? |
For sure. I think we should spin out a separate little library to host those replacements - I think they've got wider utility to support safe and efficient C++ usage across the embedded development world. (I'm honestly somewhat disappointed in the internet that I wasn't able to find such a collection already!) |
|
@willmmiles : except for the last commit that I asked copilot to remove, you you approve these changes ? |
d29785a to
0897810
Compare
0897810 to
d3bafd3
Compare
reminder ;-) |
| }; | ||
|
|
||
| /** @brief Outcome of tryParse(): the status and, when Ok, the header. */ | ||
| struct ParseResult; |
There was a problem hiding this comment.
Why forward declare this when the definition is ~20 lines down?
There was a problem hiding this comment.
because ParseResult is inside AsyncWebHeader (static ParseResult tryParse(const char *data);) and contains the resulting AsyncWebHeader.
d3bafd3 to
d7f01fa
Compare
Abort the request instead of crashing, corrupting the wire format or silently mis-parsing it when a heap allocation fails. Arduino String leaves a failed string empty, and its c_str() is null on ESP32. Sites that copied header or body data and then passed c_str() to C string functions could crash (issue #500), and sites that stored the copy in request state silently lost or corrupted data. Header parsing: - Add AsyncWebHeader::tryParse() reporting ParseStatus::NoMemory so a failed name/value copy fails the request instead of dropping the header (a lost Content-Length desynchronizes the body parser). - Scan Accept and Transfer-Encoding in place: the lowercased copies are gone, so issue #500 cannot happen and chunked framing is no longer silently missed. - Abort on failed copies into _host, _contentType, _authorization and on failed header-line/body accumulation in the plain and multipart parsers. - Write the 100-continue reply from the literal: the String copy exceeded the small-buffer size and an OOM one silently stalled the client. Responses: - Check every concat() in _assembleHead(); callers now close the connection instead of writing a truncated head (basic, abstract, WebSocket and SSE responses). - AsyncWebSocketResponse flags RESPONSE_FAILED when it cannot build Sec-WebSocket-Accept, and _sourceValid() reflects it, so the upgrade is refused rather than completing with a garbage accept header. Allocation helpers: - Use std::nothrow for response objects created by beginResponse*(), AsyncStaticWebHandler and AsyncEventSource so failures are observable instead of aborting the device; send()/_send() handle a null response. - Replace plain new with malloc in generateBasicHash() and the WebSocket printf helpers, where the null checks were dead code. Static file handler: - Stop and fall through to 404 when the path cannot be built, rather than serving the default or a wrong file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0507b51 to
94b1199
Compare
94b1199 to
ef0c963
Compare
|
LGTM though the PR is quite big and seems fixing more than one thing, so it is possible that I missed something |
ef0c963 to
35da9aa
Compare
I don't think it fixes more than needed: handling allocation failures requires to make callers and eventually client aware of them (503 or return codes or status). So yes the changes are more than just the core code doing allocations. |
|
@willmmiles : Can I let you check again please ? If the PR is good for you, then you can merge. Thanks :-) |
739868d to
bbf6947
Compare
ad540a8 to
fc6f52d
Compare
…value(std::move(value)) {}
Summary
Fixes #500 and the other places where request processing broke when a heap allocation failed.
An Arduino
Stringwhose allocation fails is left empty, and itsc_str()returns NULL on ESP32. Depending on where that happened, the library crashed, silently mis-parsed the request, corrupted the HTTP wire format, or served the wrong file.Why
Issue #500 reported a crash in
_parseReqHeader(): theAcceptvalue was copied into a newString, the copy failed under heap pressure, andstrstr(NULL, ...)hard-faulted in theasync_tcptask. The same pattern existed in several other places, with different (but equally wrong) outcomes.What changed
Header parsing (
WebRequest.cpp,AsyncWebHeader.cpp)AsyncWebHeader::tryParse()returnsParseStatus::{Ok,Invalid,NoMemory}so a failed name/value copy fails the request rather than silently dropping the header. A droppedContent-LengthorTransfer-Encodingdesynchronizes the body parser.AcceptandTransfer-Encodingare now scanned in place — the lowercased copies are gone entirely, so the Null dereference in_parseReqHeader()when theAcceptheader copy fails to allocate (crash instead of rejecting the request) #500 crash cannot occur and chunked framing can no longer be silently missed._host,_contentTypeand_authorizationabort the request instead of handing handlers corrupt state._onData()and failed byte accumulation in the plain/multipart body parsers abort instead of truncating data.100 Continuereply is written straight from the literal (theStringcopy exceeded the small-buffer size, and an OOM one silently stalled the client).Responses (
WebResponses.cpp,AsyncWebSocket.cpp,AsyncEventSource.cpp)concat()in_assembleHead()is checked; on OOM the buffer is emptied and all four_respond()callers close the connection instead of writing a truncated head.AsyncWebSocketResponsesetsRESPONSE_FAILEDwhen it cannot buildSec-WebSocket-Accept, and_sourceValid()reflects it, so the upgrade is refused instead of completing with a garbage accept header.Allocation helpers
beginResponse*(),AsyncStaticWebHandlerandAsyncEventSourceusestd::nothrow, making their existing null checks live instead of unreachable (plainnewaborts the device on OOM).send()/_send()now handle a null response.generateBasicHash()and the WebSocketprintfhelpers usemallocwhere the null checks were dead code.Static file handler (
WebHandlers.cpp)canHandle()returns false so the request falls through to another handler / 404 instead of serving the default or a wrong file.Known limitation (not addressed here)
std::list::emplace_back(headers, params),std::vector,std::make_shared(WebSocket message buffers) and thecbufallocated insideAsyncResponseStreamstill use throwing-new, so an OOM there aborts the device. Fixing that needs custom allocators/containers and is out of scope for this change.Validation
arduino-3,esp8266,raspberrypi,libretiny,arduino-2-esp-idf-log(core 2.x),no-json,regex,no-chunk-inflight,latest-asynctcp— all pass.arduino-2env itself fails before compiling any source because PlatformIO no longer serves the pinnedtool-esptoolpy@^5.3.0; this happens on unmodifiedmaintoo. Thearduino-2-esp-idf-logenv covers the same core.cpplintwith the CI filter set: 0 errors.codespell: clean. Formatted with the pinnedclang-format18.1.3.Opened as a draft for review.