sapi/cli: guard Content-Length overflow and enforce post_max_size by iliaal · Pull Request #22017 · php/php-src

iliaal · 2026-05-12T00:29:42Z

The dev server crashes when Content-Length wraps ssize_t (30+ digit value), or when a legitimately large Content-Length passes pemalloc and aborts the process.

Guard the parser's Content-Length and chunked-size accumulators against SSIZE_MAX, then reject oversize Content-Length in on_headers_complete and reply 413 with the configured post_max_size in the body.

Fixes #22003

devnexen · 2026-05-12T05:30:36Z

+# ifdef _WIN64
+#  define SSIZE_MAX _I64_MAX
+# else
+#  define SSIZE_MAX INT_MAX


more or less of the same (i.e. sizeof 4) but LONG_MAX is more accurate I think. Or even PTRDIFF_MAX.

Switched to PTRDIFF_MAX, drops the _WIN64 split.

devnexen · 2026-05-12T05:37:22Z


 	php_http_parser_init(&client->parser, PHP_HTTP_REQUEST);
 	client->request_read = false;
+	client->too_large_post = false;


too_large_post is only cleared in client_ctor. Works now since each request rebuilds the client, but if keep-alive ever reuses the struct, the flag
lingers and every follow-up request gets a bogus 413. Worth resetting it with the other per-request state, or leaving a note at the declaration

request_read, last_header_element, and current_header_* are all initialized only in client_ctor for the same reason: the client is torn down at request end. Keep-alive would need a shared per-request reset covering the lot, not just too_large_post.

LamentXU123 · 2026-05-12T15:16:14Z

The CI failure is unrelated... But the reason seems to be not hard to find as it *might* be a race condition where the address (port) is used by another test.

@iliaal You could force push this again to rerun CI :)

iliaal · 2026-05-12T15:41:25Z

The CI failure is unrelated... But the reason seems to be not hard to find as it might be a race condition where the address (port) is used by another test.

@iliaal You could force push this again to rerun CI :)

I am 91% 😆 sure it is unrelated. it is in a completely different code path.
socket_create_listen(): unable to bind to given address [48]: Address already in use
error

LamentXU123 · 2026-05-12T16:08:35Z

The CI failure is unrelated... But the reason seems to be not hard to find as it might be a race condition where the address (port) is used by another test.
@iliaal You could force push this again to rerun CI :)

I am 91% 😆 sure it is unrelated. it is in a completely different code path. socket_create_listen(): unable to bind to given address [48]: Address already in use error

Yeah and I am having a headache to fix this. I hate github CI...

arnaud-lb · 2026-07-31T16:09:06Z

-            parser->content_length *= 10;
-            parser->content_length += ch - '0';
+            if (parser->content_length > (SSIZE_MAX - (ch - '0')) / 10) {
+              goto error;


At this point the header name has been copied in php_cli_server_client_read_request_on_header_field() and will leak. We likely need to free current_header_name / current_header_value in php_cli_server_client_dtor().

Freed both in php_cli_server_client_dtor() and dropped the two asserts, which sat behind content_sender_initialized and so never fired on that teardown. Valgrind on the Content-Length overflow case: 40 bytes definitely lost before, clean after.

arnaud-lb · 2026-07-31T16:22:32Z

-        parser->content_length += c;
+        if (parser->content_length > (SSIZE_MAX - c) / 16) {
+          goto error;
+        }


I think that it's still possible to exceed post_max_size with Transfer-Encoding: chunked, as php_cli_server_client_read_request_on_body() will accumulate chunks without checking the limit.

Right. The headers-complete check reads parser->content_length, which a chunked request never sets to the body size, so the limit now applies as the chunks accumulate in on_body(). That needed a parser change as well: all three on_body call sites discarded the callback's return value. Separately, the body buffer was reserved from the declared chunk size, so 7FFFFFFFFFFFFF followed by five bytes exited the server with "Out of memory" before any limit applied; that reservation is clamped now. Covered in gh22003.phpt.

arnaud-lb · 2026-08-04T14:55:13Z

 	return 0;
 }

 static int php_cli_server_client_read_request_on_body(php_http_parser *parser, const char *at, size_t length)
 {
 	php_cli_server_client *client = parser->data;
+
+	if (SG(post_max_size) > 0 && client->request.content_len + length > (size_t) SG(post_max_size)) {


Can client->request.content_len + length overflow?

No. length is capped by the 16 KB read buffer in php_cli_server_client_read_request(), and the check keeps content_len at or below post_max_size, so the sum stays below SIZE_MAX.

Could you add an assertion and comment about this?

arnaud-lb · 2026-08-04T14:58:28Z

@@ -1386,7 +1391,9 @@ size_t php_http_parser_execute (php_http_parser *parser,

        to_read = MIN((size_t)(pe - p), (size_t)parser->content_length);
        if (to_read > 0) {
-          if (settings->on_body) settings->on_body(parser, p, to_read);
+          if (settings->on_body && 0 != settings->on_body(parser, p, to_read)) {
+            return (p - data);


Why not goto error here?

Ugh, indeed. The bare return never wrote state back to the parser, so it kept whatever it had on entry. Switched all three on_body sites and the on_headers_complete default to goto error.

arnaud-lb · 2026-08-04T15:06:06Z

 	if (!client->request.content) {
-		client->request.content = pemalloc(parser->content_length, 1);
+		size_t reserve = (size_t) parser->content_length;
+		if (SG(post_max_size) > 0 && reserve > (size_t) SG(post_max_size)) {
+			reserve = (size_t) SG(post_max_size);
+		}
+		client->request.content = pemalloc(reserve, 1);
 		client->request.content_len = 0;
 	}
 	client->request.content = perealloc(client->request.content, client->request.content_len + length, 1);


I think that the original code was not useful: The realloc is going to move the block to a smaller chunk, so the "reserve" malloc is not beneficial.

We could simplify this:

Suggested change

if (!client->request.content) {

client->request.content = pemalloc(parser->content_length, 1);

size_t reserve = (size_t) parser->content_length;

if (SG(post_max_size) > 0 && reserve > (size_t) SG(post_max_size)) {

reserve = (size_t) SG(post_max_size);

}

client->request.content = pemalloc(reserve, 1);

client->request.content_len = 0;

}

client->request.content = perealloc(client->request.content, client->request.content_len + length, 1);

client->request.content = safe_perealloc(client->request.content, 1, client->request.content_len, length, true);

Applied. That also drops the post_max_size clamp the reservation needed.

arnaud-lb

Just one comment in https://github.com/php/php-src/pull/22017/changes#r3714310817, but this looks good to me otherwise!

The dev server's HTTP parser accumulates Content-Length digits into an ssize_t without an overflow check; a 30-digit value wraps and the consumer aborts on pemalloc. Guard the decimal and chunked-size accumulators against SSIZE_MAX, then reject in on_headers_complete when the parsed length exceeds post_max_size and reply 413 with the configured limit in the body. A chunked request carries no Content-Length, so enforce the same limit as the chunks accumulate in on_body, and honour a non-zero return from that callback in the parser. The body buffer was reserved from the declared length and resized down to the bytes in hand on the next line, so a chunk header of 7FFFFFFFFFFFFF aborted the server for no gain; grow it from the received length instead. A parse error between a header name and its value left the copied name owned by nobody; release both header strings in php_cli_server_client_dtor(). Fixes phpGH-22003

arnaud-lb

Thank you!

github-actions Bot added the SAPI: cli label May 12, 2026

iliaal mentioned this pull request May 12, 2026

sapi/cli: guard Content-Length overflow and enforce post_max_size iliaal/php-src#58

Closed

devnexen reviewed May 12, 2026

View reviewed changes

iliaal force-pushed the fix/gh-22003-cli-server-content-length branch from 612aa33 to 3827cd3 Compare May 12, 2026 12:15

iliaal requested a review from devnexen May 12, 2026 13:30

iliaal force-pushed the fix/gh-22003-cli-server-content-length branch from 3827cd3 to 89565d8 Compare May 12, 2026 16:04

arnaud-lb reviewed Jul 31, 2026

View reviewed changes

iliaal force-pushed the fix/gh-22003-cli-server-content-length branch from 89565d8 to 1921a88 Compare July 31, 2026 17:07

iliaal requested a review from arnaud-lb July 31, 2026 17:39

arnaud-lb reviewed Aug 4, 2026

View reviewed changes

iliaal force-pushed the fix/gh-22003-cli-server-content-length branch from 1921a88 to ae252bf Compare August 4, 2026 15:29

iliaal requested a review from arnaud-lb August 4, 2026 15:47

arnaud-lb approved these changes Aug 4, 2026

View reviewed changes

iliaal force-pushed the fix/gh-22003-cli-server-content-length branch from ae252bf to e3ef9f4 Compare August 4, 2026 16:34

iliaal requested a review from arnaud-lb August 4, 2026 18:21

arnaud-lb approved these changes Aug 5, 2026

View reviewed changes

iliaal merged commit 454f1a6 into php:master Aug 5, 2026
17 of 18 checks passed

Uh oh!

Conversation

iliaal commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LamentXU123 commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iliaal commented May 12, 2026

Uh oh!

LamentXU123 commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb left a comment

Choose a reason for hiding this comment

Uh oh!

arnaud-lb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LamentXU123 commented May 12, 2026 •

edited

Loading