- Notifications
You must be signed in to change notification settings - Fork8k
fix: prevent C errors when using weird max_execution_time values#13942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix: prevent C errors when using weird max_execution_time values#13942
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
| // Prevent EINVAL error | ||
| if (seconds<0||seconds>999999999) { | ||
| seconds=0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe raise a warning?
withinboredomApr 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
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 onhttps://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.