Fix GH-16809: fopen HTTP wrapper timeout stream context option overflow. #16810
Conversation
| zend_string_release(transport_string); | ||
| php_url_free(resource); | ||
| return NULL; | ||
| } | ||
| #ifndef PHP_WIN32 | ||
| timeout.tv_sec = (time_t) d; | ||
| timeout.tv_usec = (size_t) ((d - timeout.tv_sec) * 1000000); |
There was a problem hiding this comment.
The size_t cast looks wrong. timeout.tv_usec is an suseconds_t which is according to POSIX:
The type suseconds_t shall be a signed integer type capable of storing values at least in the range [-1, 1000000].
Probably okay to leave this for stable branches, but I suggest to change to (suseconds_t) in master.
The (long) casts on Windows are, unfortunately, correct. From WinSock2.h:
struct timeval {
long tv_sec; /* seconds */
long tv_usec; /* and microseconds */
};There was a problem hiding this comment.
Yes I would definitively keep this existing code untouched in stable branch.
ext/standard/http_fopen_wrapper.c
Outdated
| @@ -203,6 +203,13 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, | |||
|
|
|||
| if (context && (tmpzval = php_stream_context_get_option(context, wrapper->wops->label, "timeout")) != NULL) { | |||
| double d = zval_get_double(tmpzval); | |||
|
|
|||
| if (d > (double) PHP_TIMEOUT_ULL_MAX / 1000000.0) { | |||
There was a problem hiding this comment.
Using PHP_TIMEOUT_ULL_MAX here (and maybe elsewhere) is presumptious. While that is likely good for POSIX platforms ("time_t shall be an integer type with a width (see <stdint.h> ) of at least 64 bits") (albeit not perfect since time_t could be signed), it's completely borked for Windows where long is uint32_t for x64 and x86. So for Windows, that needs to be:
if (d > (double) LONG_MAX / 1000000.0) {For stable branches, maybe define a const variable conditionally (#ifndef PHP_WIN32), and use that for the check.
There was a problem hiding this comment.
it's completely borked for Windows where long is uint32_t for x64 and x86
Oh.
The fix for phpGH-16809[1] used a way too small timeout maximum on Windows. We correct this. However, later on the timeout is applied to the current timestamp, so we would need to take that into account, but due to the elapsed time between the check and the actual network request, this could not be precise, and the resulting error message would be confusing, since after the developer would adjust the timeout to the reported maximum, the check would fail again, now reporting a lower maximum timeout. Thus we silently cap the value in `php_network_set_limit_time()` to avoid undefined behavior, and are not picky about the usec value (we just assume a second more), since there is a bigger issue, namely that we hit the Y2038 problem on Windows. [1] <php#16810>
No description provided.