Fix GH-17717: Socket maximum timeout of 2147 seconds? #17720
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
Member
Author
|
Well, let's ignore negative timeouts (and possible overflow) here; the current bug is way more important to fix. |
Member
bukka
reviewed
Dec 20, 2025
Member
bukka
left a comment
There was a problem hiding this comment.
I think it looks reasonable. But it needs rebase.
| gettimeofday(limit_time, NULL); | ||
| # ifdef PHP_WIN32 | ||
| /* cap timeout (we're not picky regarding usec) */ | ||
| if (limit_time->tv_sec > (LONG_MAX - timeout->tv_sec) + 1) { |
Member
There was a problem hiding this comment.
I'd probably remove that +1 just in case it gets called with tv_sec 0.
Member
|
@bukka since the fix never made it into 8.3, I think the original fix that caused the regression should be reverted from the 8.3 branch such that we don't ship a regression forever. |
Member
|
@ndossche Why? Regressions fixes can still be done in 8.3 (and even in 8.2), so this can be normally merged to PHP-8.3 once ready... |
Member
|
Yes but it's unfortunate it'll be delayed as it has to wait for a security release. Anyway I know that's what the rules say.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The fix for GH-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] #16810
Note that ext/standard/tests/http/gh16810.phpt fails for me on Windows with UBSan enabled for the
PHP_INT_MINcase (with or without this patch); I wonder if this actually happens on POSIX systems, too. If so, we would also need to check for a minimum timeout on all platforms; otherwise only on Windows. And frankly, I don't quite understand why we would allow negative timeout values at all.I'll have a look at how the Y2038 problem on Windows could be resolved. It's not super urgent (I don't assume that anybody uses very large timeouts anyway), but since it currently affects even 64bit platforms, that needs to be improved.