Bound the multipart parser's buffer while it waits for a boundary (#2557)

* Bound the multipart parser's buffer while it waits for a boundary

FormDataParser accumulated the entire request body whenever the declared
boundary never appeared in it. State 0 returned without erasing anything, so
the buffer grew to the full payload (100 MB by default) and buf_find rescanned
all of it on every 16 KB read. The cost grew with the square of the body size:
50 MB of '-' took 198 s of CPU on one core, and the buffer pinned the body in
memory for the whole request. One unauthenticated request was enough, and the
parser runs for any multipart request even when the handler never looks at the
parsed result.

State 0 now keeps only the last dash_boundary_crlf_.size() - 1 bytes while it
waits, which bounds both the memory and the rescan without capping how long a
preamble may be. The same 50 MB body now takes 0.14 s and the buffer stays at
one read plus the boundary. A boundary split across reads still parses, which
is what de5a255 (#2159) gave up this erase for.

State 4 buffered without bound in the same way when a boundary was followed by
neither CRLF nor "--". No further data can make such a body valid, so it now
fails right away. That is only safe because the close-delimiter branch moves to
a new state 5 that discards the epilogue: it used to stay in state 4, so an
epilogue arriving in a later read fell into this same branch. An epilogue
beginning with CRLF was then parsed as a new part and the request was rejected
with 400, which state 5 fixes as well.

Affected since v0.23.0, where de5a255 replaced the erase that had kept the
buffer in check.

* Skip buffering the multipart epilogue

Once the close delimiter has been parsed the parser is in state 5 and discards
whatever follows, but it still copied each epilogue read into the buffer before
erasing it. Return before buffering so a large epilogue spread across several
reads is dropped without being copied in at all.

* Clean up the multipart parser tests and the state 4 branch

Review follow-ups on top of the previous two commits, no behavior change.

- Move the four new tests next to the rest of MultipartFormDataTest. They
  had landed in the middle of the RedirectTest block.
- Use bind_to_any_port instead of the fixed PORT, as AGENTS.md requires for
  newly added servers. NoInitialBoundaryParsingIsNotQuadratic holds its port
  for a couple of seconds, which matters when the suite is run sharded.
- Send "Connection: close" from expect_split_multipart_ok. The server kept
  the connection alive after answering, so the response drain idled until the
  client read timeout; both tests drop from about 3s to about 0.11s.
- Drop the dead `dash_.size() > buf_size()` guard in state 4 and flatten the
  nested else. The check above it already guarantees two buffered bytes, and
  both CRLF and "--" are two bytes, so it can never fire. Removing it is what
  makes the new comment's claim readable straight off the code.

* Rename the timing test's locals to avoid a Windows macro

MSVC's <rpcndr.h>, pulled in by <windows.h>, defines `small` as `char`, so
`auto small = ...` failed to compile on the Windows jobs. Same class of
problem as the std::min / std::max collision.
This commit is contained in:
yhirose authored and GitHub committed 2026-08-25 22:55:52 -04:00
1 parent 254e576b50
commit bc58e6e9ac
2 files changed
+207 -15

No files matched your search

+29 -9
View File
@@ -8843,13 +8843,25 @@ public:
bool parse(const char *buf, size_t n, const FormDataHeader &header_callback,
const ContentReceiver &content_callback) {
// Once the close delimiter has been seen the rest of the body is epilogue
// to be discarded (RFC 2046). Drop it without buffering so a large epilogue
// spread across reads is not copied in only to be erased right away.
if (state_ == 5) { return true; }
buf_append(buf, n);
while (buf_size() > 0) {
switch (state_) {
case 0: { // Initial boundary
auto pos = buf_find(dash_boundary_crlf_);
if (pos == buf_size()) { return true; }
if (pos == buf_size()) {
// Not found yet: keep only a possible partial boundary at the tail so
// that a body which never contains the boundary cannot grow the
// buffer (and get rescanned from the start) without bound.
auto keep = dash_boundary_crlf_.size() - 1;
if (buf_size() > keep) { buf_erase(buf_size() - keep); }
return true;
}
buf_erase(pos + dash_boundary_crlf_.size());
state_ = 1;
break;
@@ -8972,18 +8984,26 @@ public:
if (buf_start_with(crlf_)) {
buf_erase(crlf_.size());
state_ = 1;
} else if (buf_start_with(dash_)) {
buf_erase(dash_.size());
is_valid_ = true;
state_ = 5;
} else {
if (dash_.size() > buf_size()) { return true; }
if (buf_start_with(dash_)) {
buf_erase(dash_.size());
is_valid_ = true;
buf_erase(buf_size()); // Remove epilogue
} else {
return true;
}
// Only CRLF (another part follows) and "--" (close-delimiter) are
// accepted after a boundary; RFC 2046 allows transport-padding in
// between, but this parser has never supported it. Either way the
// body is already destined to be rejected, so fail now instead of
// buffering the rest of it. Both are two bytes, so the check above
// already guarantees enough buffered data to decide.
is_valid_ = false;
return false;
}
break;
}
case 5: { // Epilogue
buf_erase(buf_size());
break;
}
}
}