Fixed GH-15547: curl_multi_wait expects a signed int for timeout. #15548
Conversation
confusion might come from the previous argument type. PHP expects ms so we check it fits integer boundaries before the cast. raising a warning at least for stable branches.
ext/curl/multi.c
Outdated
| @@ -187,7 +187,12 @@ PHP_FUNCTION(curl_multi_select) | |||
|
|
|||
| mh = Z_CURL_MULTI_P(z_mh); | |||
|
|
|||
| error = curl_multi_wait(mh->multi, NULL, 0, (unsigned long) (timeout * 1000.0), &numfds); | |||
| if (!(timeout >= ((double)INT_MIN / 1000.0) && timeout <= ((double)INT_MAX / 1000.0))) { | |||
| php_error_docref(NULL, E_WARNING, "timeout must be between %d and %d", (INT_MIN / 1000), (INT_MAX / 1000)); | |||
There was a problem hiding this comment.
This appears to be slightly off due to rounding. Should probably be something like (int) fceil((double) INT_MAX/1000)`, but maybe that's overkill.
And I think zend_argument_value_error() could be used here, although that wouldn't show the docref. Maybe something that should generally be addressed (unless there is already something for this).
There was a problem hiding this comment.
Oops! Of course, throwing a ValueError would be something completely different, but still this may be an option for master, and generally, zend_argument_value_error() doesn't look right, since it hasn't the docref. @Girgias and @kocsismate may have something to say about that.
There was a problem hiding this comment.
It might make sense to add support for docrefs for all argument exceptions and ZPP failures.
But the docref param is only useful for INI settings, and I don't think we throw exceptions for INI settings (yet?)
There was a problem hiding this comment.
It might make sense to add support for docrefs for all argument exceptions and ZPP failures.
There is a problem, though, since ZPP is part of the Zend engine, and php_error_docref() is part of PHP, and the engine shouldn't rely on PHP. I guess that is the reason why zend_error() has been used for this.
But the docref param is only useful for INI settings, […]
Yeah, but I didn't mean the docref param, but was referring to the general mechanism, which is nice, e.g. https://3v4l.org/deO0B.
There was a problem hiding this comment.
Ah right... I agree that Zend shouldn't rely on main/ (although I recently learnt that Zend does require ext/standard/ext/hash :|)
Maybe we should introduce a function pointer API similar to zend_autoload ?
There was a problem hiding this comment.
Oh, already forgot about the Zend -> ext/hash dependency. We should fix this (keeping BC seems to be possible, on the first glance).
And yes, a function pointer could solve the issue nicely.
And we should probably use some tooling to detect such circular dependencies.
ext/curl/tests/gh15547.phpt
Outdated
| var_dump(curl_multi_select($mh, 1000000)); | ||
| ?> | ||
| --EXPECTF-- | ||
| Warning: curl_multi_select(): timeout must be between %s and %s in %s on line %d |
There was a problem hiding this comment.
nit:
| Warning: curl_multi_select(): timeout must be between %s and %s in %s on line %d | |
| Warning: curl_multi_select(): timeout must be between %d and %d in %s on line %d |
| @@ -187,7 +187,13 @@ PHP_FUNCTION(curl_multi_select) | |||
|
|
|||
| mh = Z_CURL_MULTI_P(z_mh); | |||
|
|
|||
| error = curl_multi_wait(mh->multi, NULL, 0, (unsigned long) (timeout * 1000.0), &numfds); | |||
| if (!(timeout >= 0.0 && timeout <= ((double)INT_MAX / 1000.0))) { | |||
| php_error_docref(NULL, E_WARNING, "timeout must be between 0 and %d", (int)ceilf((double)INT_MAX / 1000)); | |||
There was a problem hiding this comment.
Sorry, the ceilf() is still not quite correct. Maybe we should actually output a float value? Or maybe leave as is, and call it a day. It's quite unlikely that a user specifies a timeout of almost a month.
|
Is there anything need to be done, in the stable branches ? |
|
I think this is good as is, but it would be great if @adoy could review (wrote the code, is code owner, and RM). |
throws a ValueError on timeout overflow.
throws a ValueError on timeout overflow. close GH-15594
confusion might come from the previous argument type. PHP expects ms so we check it fits integer boundaries before the cast. Raising a warning at least for stable branches.