Fix GH-17717: Socket maximum timeout of 2147 seconds? #17720
Conversation
This file contains 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>
|
Well, let's ignore negative timeouts (and possible overflow) here; the current bug is way more important to fix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.