[binaryfire]

Large multipart uploads are parsed while the body is received, and Swoole\Http\Request::parse() accepts body data in later calls. Part headers split between reads were rejected or rebuilt from pointers into an earlier read, and a Content-Disposition value larger than the fixed scratch buffer could overflow that buffer.

This change:

• accumulates multipart header names and values until each header is complete, within the existing limits;
• rejects a declared body that ends before its multipart parser completes;
• lets valid header and closing-boundary fragments continue across reads;
• resets parser state when a repeated Content-Type replaces an earlier value;
• reports multipart callback failures with a stable error reason; and
• closes unfinished wrapper upload handles before their paths are removed.

Multipart parser initialization is no longer compiled out on Windows. The Windows workflow validates that this code builds, but it does not run the focused core tests or swoole_http_server PHPTs. The behavior regressions run in the Unix jobs.

This PR overlaps the multipart preprocessing and wrapper ownership paths changed by #6161 and #6162. Whichever change is merged second will need to preserve the other one's behavior when the maintained branches are merged forward.

Hypervel regression: test. This currently fails on affected Swoole versions.

[codecov]

Codecov Report

❌ Patch coverage is 76.19048% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.15%. Comparing base (419ca54) to head (1f9a42a).

Files with missing lines	Patch %	Lines
src/protocol/http.cc	70.31%	19 Missing ⚠️
src/wrapper/http.cc	84.00%	8 Missing ⚠️
src/server/port.cc	50.00%	2 Missing ⚠️
thirdparty/multipart_parser.c	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6154      +/-   ##
==========================================
+ Coverage   84.10%   84.15%   +0.05%     
==========================================
  Files         114      114              
  Lines       18523    18607      +84     
  Branches     3292     3307      +15     
==========================================
+ Hits        15578    15658      +80     
- Misses       2945     2949       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[NathanFreeman]

Swoole's asynchronous server waits until the entire request data has been received before it starts parsing. If the reception times out, or if the packet size exceeds the configured limit, the request is discarded directly and will not be processed.

[binaryfire]

@NathanFreeman Correct for normal request headers: the async server receives the complete request before the worker parses it.

This PR covers other cases that are parsed in parts:

• Swoole\Http\Request::parse() can be called multiple times.
• The coroutine HTTP client can receive response trailers across multiple reads.
• Chunked multipart bodies are passed to the multipart parser one chunk at a time.
• Large multipart uploads are parsed while the body is still being received. This happens in ListenPort::readable_callback_http(), which calls parse_multipart_data().

The cleanup and duplicate Content-Type fixes are separate from fragmentation. I've updated the PR description to make this clearer.

[NathanFreeman]

This test can pass without these fixes.

[binaryfire]

@NathanFreeman You are right. This HTTP/2 test does not depend on the fix, so I have removed it.

[NathanFreeman]

I understand that this test is designed to verify the combination of Transfer-Encoding: chunked and multipart/form-data. However, I believe this compatibility is unnecessary. For small files, simply submitting them directly via multipart/form-data is sufficient. For large files, a more reliable approach is chunked file upload — where the frontend splits the file into smaller pieces, sends each piece as an independent multipart/form-data request, and the backend reassembles these pieces after receiving all of them.

Transfer-Encoding: chunked is essentially a single, continuous HTTP request. If any minor network fluctuation or interruption occurs during this potentially minutes-long streaming transmission, the entire TCP connection will be terminated, and all the data already transmitted — say, 99% of the file — will be lost entirely, requiring a full retransmission from the very beginning.

[binaryfire]

@NathanFreeman I understand. Splitting large files into separate requests is more reliable.

This test is not intended to recommend one long chunked upload. It uses behavior that Swoole currently accepts to make the multipart parser receive a header in more than one part.

Do you mean Swoole should not support Transfer-Encoding: chunked together with multipart/form-data, or only that it should not be recommended for large file uploads?

If this combination is not supported, I will remove these tests. If it is supported, the parser should still handle the request correctly.

[NathanFreeman]

Here, 0\r\n indicates the end of the entire message.

[binaryfire]

@NathanFreeman In HTTP/1.1 chunked encoding, 0\r\n marks the last data chunk. Trailer fields may follow it, and the message ends after the empty line following the trailers:

0\r\n
X-Trailer: value\r\n
\r\n

This test checks a trailer field received in multiple parts. I added Trailer: X-Trailer to the initial response headers to make the intent clearer.

[NathanFreeman]

This function is unnecessary because when receiving HTTP messages, the size of the request headers is already checked.

swoole-src/ext-src/swoole_http_client_coro.cc

Lines 1507 to 1511 in cffd0d0

	if (buffer->length == buffer->size) {
	swoole_error_log(SW_LOG_TRACE, SW_ERROR_HTTP_INVALID_PROTOCOL, "Http header too large");
	socket->set_err(SW_ERROR_HTTP_INVALID_PROTOCOL);
	break;
	}

[binaryfire]

@NathanFreeman Agreed. I’ve removed this helper and reverted the coroutine HTTP client changes.

[NathanFreeman]

This approach does not have any significant advantages over the original.

[binaryfire]

@NathanFreeman Agreed. I’ve restored the original client parser handling here.

[NathanFreeman]

I think this is worth discussing. Currently, Nginx does not forcibly return an error when it encounters duplicate request headers. Swoole's HTTP and HTTP2 servers, on the other hand, handle duplicate request headers by having the latter override the former.

[binaryfire]

@NathanFreeman Agreed. I’ve removed the rejection. The later Content-Type now also replaces the body parser state, so multipart and URL-encoded parsing follow the same last-wins behavior as the header array.

[NathanFreeman]

This can be left as is. The code above already explains the error message.

[binaryfire]

@NathanFreeman Agreed. I’ve restored the original error handling here.

[NathanFreeman]

This check is unnecessary. If we've reached this point, the request header size must already be within the limit.

[binaryfire]

@NathanFreeman Agreed. I’ve removed this check and reverted the client path.

[NathanFreeman]

swoole-src/src/server/port.cc

Lines 513 to 521 in cffd0d0

	if (request->method == 0 && request->get_protocol() < 0) {
	if (!request->excepted && buffer->length < SW_HTTP_HEADER_MAX_SIZE) {
	return SW_OK;
	}
	swoole_error_log(SW_LOG_TRACE,
	SW_ERROR_HTTP_INVALID_PROTOCOL,
	"Bad Request: unknown protocol" CLIENT_INFO_FMT,
	CLIENT_INFO_ARGS);
	goto _bad_request;

The HTTP request header size has already been checked here.

[binaryfire]

@NathanFreeman Agreed on the additional size check; I’ve removed it. I kept owned header strings because Swoole\Http\Request::parse() can receive separate caller-owned strings across calls, and that path doesn’t go through ListenPort.

[NathanFreeman]

There's no need to handle the success case separately here.

[binaryfire]

@NathanFreeman Agreed. I’ve restored the original error handling here.

[NathanFreeman]

This is Swoole::String, not std::string.

[binaryfire]

@NathanFreeman Correct. I’ve restored Swoole::String. ContextImpl owns it and releases it with the multipart parser.

[NathanFreeman]

swoole-src/src/protocol/http.cc

Lines 812 to 827 in cffd0d0

	void Request::destroy_multipart_parser() {
	auto tmp_buffer = buffer_;
	delete tmp_buffer;
	buffer_ = form_data_->multipart_buffer_;
	form_data_->multipart_buffer_ = nullptr;
	if (form_data_->multipart_parser_->fp) {
	fclose(form_data_->multipart_parser_->fp);
	unlink(form_data_->upload_tmpfile->str);
	}
	multipart_parser_free(form_data_->multipart_parser_);
	form_data_->multipart_parser_ = nullptr;
	delete form_data_->upload_tmpfile;
	form_data_->upload_tmpfile = nullptr;
	delete form_data_;
	form_data_ = nullptr;
	}

This piece of code is responsible for cleaning up mt_parser.

[binaryfire]

@NathanFreeman The linked cleanup handles Request::form_data_->multipart_parser_. The wrapper uses a separate ContextImpl::mt_parser, so that cleanup doesn’t reach it. I’ve kept the wrapper cleanup and made it close any open upload stream before freeing the parser.

[NathanFreeman]

It is recommended not to support the combined use of Transfer-Encoding: chunked and multipart/form-data, as well as the trailer fields feature (Trailers). The main reasons are that mainstream browsers have limited compatibility with these protocol features, and in mature industry practice, they are typically handled by reverse proxy layers such as Nginx, so there is no need for the application layer to implement them redundantly.

[binaryfire]

@NathanFreeman Agreed. I’ve removed response-trailer support and the chunked multipart tests. The PR is now limited to fragmented Request::parse() input, large-upload preprocessing, and multipart cleanup. I’ve updated the description to match.

[NathanFreeman]

I don't see this as an improvement over the original code. We're still converting std::string to const char * at the end of the day, and the at parameter received by both http_request_on_header_field and http_request_on_header_value is already a complete segment — there's no fragmentation issue there.

[binaryfire]

@NathanFreeman at contains the data available for the current callback. It is not guaranteed to contain the complete header value.

When an llhttp_execute() input buffer ends, llhttp delivers the current part of the value. A later call continues the same value. The completion callback is the point where the whole value is known.

Previously, each callback replaced the saved pointer and length. The new code accumulates the parts until completion.

[NathanFreeman]

Why was this changed to std::string here?

[binaryfire]

@NathanFreeman The part's Content-Type can be parsed in one call, while its Content-Disposition is parsed in a later call. The content type is read at that later point, so it cannot remain a pointer into the earlier input buffer.

The regression test splits the input between those two headers.

[NathanFreeman]

Why is this function needed?

[binaryfire]

@NathanFreeman multipart_parser_free() only frees the parser allocation. It does not close the open upload file.

This helper closes the file and releases the parser and its buffer during request cleanup, message completion, and parser replacement.

[NathanFreeman]

This also has the same issue as http_request_on_header_value_complete.

[binaryfire]

@NathanFreeman The multipart parser already allows a field name to arrive in multiple pieces. This change handles field values the same way.

I removed the incomplete-value error because the end of one parser call does not mean the header has ended. When the HTTP message actually completes, the final parser state is checked and an unfinished header is still rejected.

The core tests cover split headers, successful completion, and a message that ends with an incomplete header.

[NathanFreeman]

I've reviewed this PR, and the main point of contention lies in how HTTP messages are received and parsed. The PR author's approach is to parse the data incrementally using llhttp and the multipart parser as it is being received. However, the actual behavior of the underlying layer is as follows:

Asynchronous server: ensures that a complete HTTP message is received before triggering parsing;

Coroutine server: ensures that the headers are fully received, while the remaining data is parsed incrementally as it arrives. Even if a header key is incomplete, the parser will pause and wait for more data to arrive before triggering the corresponding callback.

If the underlying layer does not actually work this way, then the PR author needs to provide a reproducible code example to demonstrate that the current logic does have an issue.

[NathanFreeman]

My understanding of the C++ HTTP wrapper module is limited to its use in kernel testing, and I don't believe it's involved in production environments. However, to be on the safe side, I'd like to ask @matyhtf to confirm whether there are any other hidden call paths or indirect usages we should be aware of.

[NathanFreeman]

I don't have any objections to the fix for init_multipart_parser.

[NathanFreeman]

The fix for Swoole\Http\Request::parse() works by copying and concatenating the incoming data into an internal buffer, completely isolating the parser from external data. This avoids the risk of crashes caused by the external data's lifecycle ending.

static PHP_METHOD(swoole_http_request, parse) {
    HttpContext *ctx = http_request_get_and_check_context(ZEND_THIS);
    if (UNEXPECTED(!ctx) || ctx->completed) {
        RETURN_FALSE;
    }

    char *str;
    size_t l_str;

    ZEND_PARSE_PARAMETERS_START(1, 1)
    Z_PARAM_STRING(str, l_str)
    ZEND_PARSE_PARAMETERS_END_EX(RETURN_FALSE);

    if (Z_TYPE(ctx->request.zdata) != IS_STRING) {
        ZVAL_STRINGL(&ctx->request.zdata, str, l_str);
    } else {
        size_t len = Z_STRLEN(ctx->request.zdata) + l_str;
        zend_string *new_str = zend_string_alloc(len + 1, false);
        memcpy(new_str->val, Z_STRVAL(ctx->request.zdata), Z_STRLEN(ctx->request.zdata));
        memcpy(new_str->val + Z_STRLEN(ctx->request.zdata), str, l_str);
        new_str->val[len] = 0;
        new_str->len = len;
        zval_dtor(&ctx->request.zdata);
        ZVAL_STR(&ctx->request.zdata, new_str);
    }

    RETURN_LONG(ctx->parse(str, l_str));
}

[binaryfire]

@NathanFreeman You are right that the normal async and coroutine server paths do not expose this header-fragmentation case.

There are two incremental paths: the public Swoole\Http\Request::parse() API and the large-upload preprocessor. Both can call the parser multiple times for one request, so a callback can receive only the part available in the current call.

The corrected PHP test sends one complete form part, then splits the next part's header across two writes. I ran it against the PR base. There it fails because the server answers 400 Bad Request. With this change it answers 200 OK and returns the field value and the 131072-byte file.

The C++ test has a different purpose. It checks that the large-upload preprocessor rejects an accumulated part header after it crosses the size limit and does not let the request reach the worker. It passes on both versions because the old parser also rejects the fragmented value, so I am not using it as proof of this fix. Its worker-response check makes sure the test cannot silently stop entering the preprocessor path.

If a client stops sending before the multipart header is complete, the request is not dispatched and no response is sent. The connection remains open until the client closes it or the configured idle timeout expires. If the HTTP message actually completes with an unfinished part header, it is rejected.

While checking your comments, I also found the same issue in incremental URL and form-body parsing. Those are now fixed, and the tests parse the same request at every split point and one byte at a time.

For Request::parse(), the old code copied the input into request.zdata, but passed the original str buffer to the parser. Also, zdata is replaced on every call. A callback pointer saved during one call could therefore become invalid during the next call. Data that must survive between calls is now stored in owned strings.

I kept the init_multipart_parser() check in the large-upload preprocessor. An invalid multipart boundary can make initialization fail, but this caller previously ignored the result and continued with a null parser. The extension and HTTP/2 callers already check the result.

You are also right that the C++ wrapper is currently used only by the core tests. I removed its wrapper-only behavior changes. The remaining changes are required by the shared parser interface and parse_cookie return value, and to initialize and release its own parser state safely. The parse_cookie length check remains because the production large-upload preprocessor also calls it.

[NathanFreeman]

I have already understood the details of this PR. The modification to Swoole\Http\Request::parse() is actually unnecessary.

The reason is that both the HTTP asynchronous server and the HTTP coroutine server have already handled the issue of byte-by-byte reception at the underlying level, so developers do not need to intervene additionally.

As for the use case of Request::parse(), the only scenario I can think of is when manually receiving HTTP messages in a TCP server, where this method might be needed. However, this approach requires the user to ensure that the complete HTTP message has been received, and then pass it to Request::parse() for parsing. In other words, this method is only applicable to this specific scenario where developers need to control data integrity themselves.

<?php
// server
use Swoole\Http\Server;
use Swoole\Http\Request;
use Swoole\Http\Response;

$http = new Server('127.0.0.1', 9501);

$http->on('start', function (Server $server) {
    echo "Swoole http server is started at http://127.0.0.1:9501\n";
});

$http->on('request', function (Request $request, Response $response) {
    var_dump($request);
    $response->header('Content-Type', 'text/plain');
    $response->end('Hello World');
});

$http->start();

<?php
// client
use function Swoole\Coroutine\run;
use Swoole\Coroutine\Client;


$fragmented = "POST /upload?first=one&second=two HTTP/1.1\r\n";
$fragmented .= "Host: 127.0.0.1\r\n";
$fragmented .= "Content-Type: application/x-www-form-urlencoded\r\n";
$fragmented .= "Authorization: Bearer fragmented\r\n";
$fragmented .= "X-Custom-Header: custom value\r\n";
$fragmented .= "Upgrade-Insecure-Requests: \r\n";
$fragmented .= "User-Agent: fragmented-agent\r\n";
$fragmented .= "Cookie: first=one; second=two\r\n";
$fragmented .= "Content-Length: 0\r\n\r\n";

run(function() use ($fragmented){
    $client = new Client(SWOOLE_SOCK_TCP);
    if (!$client->connect('127.0.0.1', 9501, 0.5)) {
        exit("error code: {$client->errCode}\n");
    }
    foreach (str_split($fragmented) as $byte) {
            $client->send($byte);
            usleep(100);
    }
    $response = $client->recv();
    echo "response: {$response}\n";
    $client->close();
});

[binaryfire]

@NathanFreeman I agree that the built-in asynchronous and coroutine HTTP servers handle their own receive buffering. The public Request::parse() API seems to have a separate use, though, and it predates this PR.

The coroutine server test create_response.phpt creates one Request and feeds it each $conn->recv() chunk in a loop until the request is complete. create_request.phpt splits one request across several parse() calls in the same way.

This change does not add that usage. It makes it safe at every input boundary, because llhttp can report a URL, header, or body as more than one piece when the input ends inside it.

If parse() should require a complete message, I will remove that part from this PR. Before changing the existing examples, I would need to understand the supported replacement: a raw Coroutine\Server handler receives socket chunks and has no HTTP framing option, so it cannot know that a request is complete without parsing HTTP itself. Should that example use Swoole\Coroutine\Http\Server instead?

If incremental use should remain supported, this change makes it correct. Which behavior should this API support?

The large-upload preprocessing fix is separate from this question. It uses a normal Swoole\Http\Server with no Request::parse() involved, and upload_fragmented_multipart_header.phpt returns 400 on the PR base and 200 with this change.

[NathanFreeman]

The large upload preprocessing is a bug, and I am still carefully studying the details of this part.

[NathanFreeman]

Request::parse() should keep its existing behavior. When using a TCP server to simulate an HTTP server, it is the user's responsibility to ensure the integrity of the HTTP message. The reason Request::parse() supports incremental parsing is that when the user receives a complete HTTP request header, they can first call this function to parse out the Content-Length, determine how much more data needs to be received, and then pass the received request body data to Request::parse() for further parsing.

Parsing byte by byte is a lazy approach that results in multiple memory allocations and copies, which is not the intended design goal of this API.

The create_request.phpt test case itself ensures that the received data is a complete HTTP request, so this incremental receiving step is omitted.

[binaryfire]

@NathanFreeman I updated the PR to follow the contract you described.

Request::parse() now expects the complete HTTP request header in the first call. I removed the URL and header accumulation, so the request line and headers now use the original input directly, without allocating extra strings. I also removed the byte-by-byte tests.

The opening case in create_request.phpt split the request at a random offset between 100 and 600 bytes, which could split inside a header. It now passes the complete request header in one call.

Request body data may still be passed in later calls. The parser reads that data from the buffer owned by the request, so URL-encoded POST data stays valid across calls. The regression sends one complete header first, then the body in later calls.

The multipart changes remain for this body-after-header use and for large upload preprocessing. The upload regression sends one complete form part, then splits the next part header across two reads, once inside the header name and once inside its value. Both fail on the base branch; with this change the server returns the parsed field and the uploaded file.

I also updated the PR title and description to match the final scope.

[binaryfire]

Follow-up: I’ve added the missing error reason for multipart callback failures. These failures already rejected the request, but the shared parser left its error reason empty, so the HTTP paths logged a blank reason. The parser now reports callback error, with focused coverage for both callback paths; request status and parsing behavior are unchanged.

[matyhtf]

There are several correctness gaps in the final head that need to be addressed before this large parser change can merge:

1. src/wrapper/http.cc does not actually implement the stated last-value-wins behavior for repeated Content-Type. Unlike the extension and Request::parse_header_info() paths, the wrapper does not clear ctx->post_form_urlencoded and does not call destroy_multipart_parser() before interpreting every replacement value. Consequently multipart/form-data -> text/plain still parses the body with the old multipart parser, and application/x-www-form-urlencoded -> text/plain leaves form parsing enabled. Please reset both derived states first, then apply the new value, and add wrapper-level tests for both orders (including a non-form final value).

2. The large-upload preprocessor still cannot detect that a declared body ended in an incomplete multipart header. After parse_multipart_data() consumes the final bytes, tried_to_dispatch remains false and readable_callback_http() simply waits for another read. It does not track the number of original body bytes already consumed, so when the client has sent exactly Content-Length both sides can wait indefinitely; closing the client only closes the connection without the intended 400 response. Please track raw body progress and reject !multipart_parser_is_complete() once the declared body length has been received. Add a >64 KiB/preprocessor regression whose final part header is truncated.

3. The C++ wrapper's cleanup closes mt_parser->fp but does not unlink a temporary upload file created before a malformed/incomplete request fails. Since impl.parse() returns false, the request callback never receives ctx.files, and destroying the map only discards the pathname. Please remove wrapper-owned temporary files on parse failure (while preserving the valid-request ownership contract) and test that cleanup.

4. This branch is based on the old refactory-http-coro-client line rather than current master. Its latest Windows job does not compile at all (the failures are from obsolete Win32 compatibility code), so the claim that multipart initialization now works on Windows has not been validated. Please rebase onto the current Windows-capable master and rerun the focused core/PHPT cases on Windows as well as Linux/macOS.

The fragmented header accumulation and callback-error reporting are otherwise heading in the right direction; the new Linux PHPTs passed in the latest run.

[binaryfire]

@matyhtf Done.

1. The C++ wrapper now clears both derived form states before applying each replacement Content-Type. The focused wrapper test covers both multipart/form-data -> text/plain and application/x-www-form-urlencoded -> text/plain.
2. The large-upload path now tracks consumed body bytes and returns 400 Bad Request if the declared body ends before multipart parsing completes. I also removed the old \r\n\r\n wait, since the parser now preserves split header fields and values itself. The regression covers a truncated part header and a valid closing boundary split between reads.
3. On a wrapper parse failure, the multipart parser is destroyed before Context removes its tracked upload paths, so Windows closes the file before unlinking it. Linux already removes open files, while the Windows workflow runs neither core tests nor the swoole_http_server PHPTs, so there isn't a discriminating automated regression for that ordering in the current CI setup.
4. I rebuilt the PR on current master, excluding the unrelated changes from the old base, and retargeted it to master. Multipart initialization is no longer compiled out on Windows. The Windows job can validate compilation; the focused behavior cases run in the Unix jobs.

I've updated the title and description as well. Please take another look when you have a moment.

[binaryfire]

Rebased onto current master.

#6218 added a Request::parse() case that splits a header value across two calls. With this PR's previous parse() change, the header name saved during the first call pointed into an input buffer that the second call frees, so that test read freed memory. #6224 fixes Request::parse() input lifetime on 6.1 for URLs, headers, and bodies, so I removed the parse() change and its URL-encoded body test from this PR. The multipart changes don't depend on it. I've updated the description to match.

[matyhtf]

The changes from #6161, #6162, and #6154 have now been integrated, refined, and squash-merged into master via #6309.

Merged commit: 0306f53

The integration includes additional regression tests and fixes for multipart parsing, temporary-file ownership and cleanup, and Windows compatibility. The latest branch CI passed before merging.

This PR is superseded by the merged integration. The combined refactor will not be backported wholesale to the 6.1 or 6.2 bug-fix branches. Thank you for the contribution.