Integer overflow in curl_multi_select

[Jumping-Beaver]

Description

In this code line, the timeout is casted to unsigned long:

php-src/ext/curl/multi.c

Line 190 in f952263

error = curl_multi_wait(mh->multi, NULL, 0, (unsigned long) (timeout * 1000.0), &numfds);

But the function expects a signed int, see https://curl.se/libcurl/c/curl_multi_wait.html

In consequence, passing a large timeout to the PHP function curl_multi_select causes undefined behavior according to the C standard. Usually it would it cause 100% CPU usage.

PHP Version

PHP 8.3.10

Operating System

No response

[cmb69]

Thanks for reporting!

Indeed, that cast is wrong, but most important seems that we should check that timeout is in the permissible range before even calling the function, and throw a ValueError otherwise (not sure if throwing is okay for stable branches, though).

[Jumping-Beaver]

A comment on the patch.

When the function returns -1, it would be good to always set some Curl error. Otherwise the caller cannot know to which Curl function call the error fetched with curl_multi_errno refers.

Specifically, we can use SAVE_CURLM_ERROR(mh, CURLM_BAD_FUNCTION_ARGUMENT) because this error code is also set in Curl when the timeout is negative: https://github.com/curl/curl/blob/df15d9ff26170125cbaa4d9283f733a2bb35fa20/lib/multi.c#L1288

As a code simplification, we could also assert timeout >= 0.0 because negative values are disallowed anyway. (Curl should better use the unsigned int type here.)

[devnexen]

sure, fair point for setting curl error.

[novotnicek]

we have issue with <b>Warning</b>: curl_multi_select(): timeout must be between 0 and 2147484 in <b>/apps/symfony-app/vendor/symfony/http-client/Response/CurlResponse.php</b> on line <b>342</b><br /> in response body.

but if display_errors = Off, then we expect this message in logs, nope in response body.

i don't know if it's depended, but in php-8.4.0RC2 is used zend_argument_value_error() instead of php_error_docref().

[devnexen]

It s documented on the UPGRADING notes.