fix: prevent C errors when using weird max_execution_time values #13942
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
staabm
reviewed
Apr 11, 2024
| @@ -1518,6 +1518,11 @@ static void zend_set_timeout_ex(zend_long seconds, bool reset_signals) /* {{{ */ | |||
| struct itimerval t_r; /* timeout requested */ | |||
| int signo; | |||
|
|
|||
| // Prevent EINVAL error | |||
| if (seconds < 0 || seconds > 999999999) { | |||
| seconds = 0; | |||
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.
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.
|
Thank you! |
|
Nice find @dunglas |
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.
Calling
set_time_limit()or settingmax_execution_timeto a negative value or to a value superior to 999,999,999 can trigger C errors: dunglas/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.