fix: prevent C errors when using weird max_execution_time values#13942
Merged
arnaud-lb merged 1 commit intophp:PHP-8.2from Apr 16, 2024
Merged
Conversation
staabm
reviewed
Apr 11, 2024
|
|
||
| // Prevent EINVAL error | ||
| if (seconds < 0 || seconds > 999999999) { | ||
| seconds = 0; |
Member
There was a problem hiding this comment.
FWIW, other SAPI's don't appear to raise a warning and a lot of people make warnings exceptions. But yeah, I agree and that's the entire purpose of the discussion on internals....
For now, it is probably better to conform to existing behavior vs. introducing new behavior.
Member
There was a problem hiding this comment.
I agree. On a second look, this is just making zend-max-execution-timers consistent with other timeout implementations. set_time_limit(-1) disables the timeout (probably by accident), and people rely on it: https://github.com/search?q=%22set_time_limit%28-1%29%22&type=code.
Member
|
Thank you! |
Contributor
|
Nice find @dunglas |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Calling
set_time_limit()or settingmax_execution_timeto a negative value or to a value superior to 999,999,999 can trigger C errors: php/frankenphp#713 / https://linux.die.net/man/2/setitimerThis patch normalizes such values as 0.
@withinboredom raised the issue on https://externals.io/message/123108, and we may indeed correctly specify this behavior, but in the meantime, we should at least not throw a C error, which is inconsistent with what is done on other platforms.