Skip to content

ext/standard: reject a dechunk chunk size that overflows size_t#22786

Open
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:std-ss007-dechunk-size-overflow
Open

ext/standard: reject a dechunk chunk size that overflows size_t#22786
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:std-ss007-dechunk-size-overflow

Conversation

@iliaal

@iliaal iliaal commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

php_dechunk() accumulated the hex chunk size with chunk_size * 16 + digit and never checked the multiply. A size of 10000000000000000 is 2^64, which wraps to 0, and 0 reads as the terminating chunk, so the filter stops and drops the body it was handed:

// before
string(0) ""                     // 10000000000000000\nBODYDATA\n0\n
// after
string(13) "0\nBODYDATA\n0\n"    // CHUNK_ERROR, passed through like other malformed sizes

The guard is hoisted out of the three digit branches rather than repeated in each, which needs the digit value computed first. SIZE_MAX / 16 is sufficient for the add as well: the largest value that survives it is SIZE_MAX / 16, and SIZE_MAX / 16 * 16 + 15 is exactly SIZE_MAX.

php_dechunk() accumulated the hex chunk size with chunk_size * 16 + digit and
never checked the multiply, so a size of 2^64 wrapped to 0. A zero size reads
as the terminating chunk, so the filter stopped there and dropped the body it
had been handed. Error out instead, as the other malformed-size cases already
do.
@Sjord

Sjord commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Yes, that's indeed a possible overflow. How did you stumble upon this?

I don't like the test very much; it tests for the new behavior but not for correct behavior. But I guess it's the best we can do currently.

Have you considered using ckd_mul / __builtin_mul_overflow?

@iliaal

iliaal commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

On the test: agreed, it only pins the reject-and-passthrough behavior. The SIZE_MAX/16 boundary isn't reachable without a ~2^60-byte body, so the 5\nhello case is the practical valid-path check.

On ckd_mul / __builtin_mul_overflow: both work here. I kept SIZE_MAX/16 as a portable one-liner (no builtin or configure probe), matching an existing idiom in the tree like the LONG_MAX/86400 bound in ext/openssl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants