Fix GH-20601: ftp_connect() timeout argument overflow. #20603
Conversation
| struct timeval *timeout) | ||
| { | ||
| gettimeofday(limit_time, NULL); | ||
| const double timeoutmax = (double) PHP_TIMEOUT_ULL_MAX / 1000000.0; |
There was a problem hiding this comment.
I'm just checking the whole logic here and I wonder why PHP_TIMEOUT_ULL_MAX is unsigned int 64? The time_t is signed int 64 (or 32 on 32bit arch) if I'm not mistakend. And in this case this is later converted to int (see php_pollfd_for and mainly php_tvtoto). So what am I missing?
There was a problem hiding this comment.
Yes it s unsigned but my sense since it s that way because this constant is never used directly but converted from microseconds value to seconds (so yes unsigned int64 > double > time_t or int).
There was a problem hiding this comment.
But how is this covering the lower type overflows? I mean if it gets converted in php_tvtoto, then it will still overflow on conversion to int, right?
main/network.c
Outdated
| gettimeofday(limit_time, NULL); | ||
| const double timeoutmax = (double) PHP_TIMEOUT_ULL_MAX / 1000000.0; | ||
|
|
||
| if (limit_time->tv_sec > (timeoutmax - timeout->tv_sec)) { |
main/network.c
Outdated
| php_network_set_limit_time(&limit_time, &working_timeout); | ||
| if (UNEXPECTED(php_network_set_limit_time(&limit_time, &working_timeout) == FAILURE)) { | ||
| php_network_freeaddresses(psal); | ||
| zend_value_error("timeout value overflow"); |
There was a problem hiding this comment.
I'm not sure about throwing error here even though it already triggers normal errors in some cases.
Why is this even checked here and not in the actual function as it has been case before?
main/network.c
Outdated
| if (limit_time->tv_sec >= (timeoutmax - timeout->tv_sec)) { | ||
| zend_value_error("timeout value overflow"); | ||
| return FAILURE; | ||
| } |
There was a problem hiding this comment.
I would rather have this be an assertion and make it the responsibility of the calling code to ensure this is respected. So move the check to ftp_connect(), especially as you get better error messages and you can pin point to the exact argument is causing an issue.
There was a problem hiding this comment.
ok done, note that I purposely kept separated the check for <= 0 and the new one since it s stable branch.
| #if HAVE_GETTIMEOFDAY | ||
| php_network_set_limit_time(&limit_time, &working_timeout); | ||
| if (UNEXPECTED(php_network_set_limit_time(&limit_time, &working_timeout) == FAILURE)) { | ||
| php_network_freeaddresses(psal); | ||
| return -1; | ||
| } | ||
| #endif |
| #if HAVE_GETTIMEOFDAY | ||
| php_network_set_limit_time(&limit_time, &working_timeout); | ||
| if (UNEXPECTED(php_network_set_limit_time(&limit_time, &working_timeout) == FAILURE)) { | ||
| error = ERANGE; | ||
| ret = -1; | ||
| goto ok; | ||
| } | ||
| #endif |
| --SKIPIF-- | ||
| <?php | ||
| if (PHP_INT_SIZE != 8) die("skip: 64-bit only"); | ||
| if (PHP_OS_FAMILY === 'Windows') die("skip not for windows"); | ||
| ?> |
There was a problem hiding this comment.
This an issue that cannot appear on 32 bit nor Windows?
There was a problem hiding this comment.
yes this test "failed" for both earlier.
3 participants
No description provided.