ext/session: Fix cache_expire ini overflow/underflow. #16445
Conversation
Setting with PHP_INT_MIN/PHP_INT_MAX lead to these. ``` ext/session/session.c:1181:37: runtime error: signed integer overflow: -9223372036854775808 * 60 cannot be represented in type 'long int' ext/session/session.c:1181:37: runtime error: signed integer overflow: 9223372036854775807 * 60 cannot be represented in type 'long int' ```
devnexen
force-pushed
the
cache_expire_oflow
branch
5 times, most recently
from
0c404c0 to
8200b40
Compare
devnexen
force-pushed
the
cache_expire_oflow
branch
from
8200b40 to
daf1cf6
Compare
ext/session/session.c
Outdated
| if (v < 0 || v > maxexpire) { | ||
| return SUCCESS; | ||
| } |
There was a problem hiding this comment.
Surely this should be return FAILURE ?
There was a problem hiding this comment.
In fact I applied the same "policy" as for cookie_lifetime as to not disturb things for stable branches but I can return FAILURE sure.
There was a problem hiding this comment.
I think this strategy makes sense.
However, the approach appears to be insufficient. For 32bit architectures we have ((ZEND_LONG_MAX / 2) / 60) - 1;. Asssuming that time_t is a signed 32bit, that can still overflow: let's say the current timestamp is 1728992731; the max value would be 7456539. Then we do tv.tv_sec + PS(cache_expire) * 60, which results in signed overflow. For our Windows 32bit builds, time_t is long which is a signed 32bit value on LLP64.
It seems to me that we need to do the overflow check when we're actually calculating the time (maybe instead of failing, just clamping the value). Rejecting very large values in the INI modifcation handler makes sense, but doesn't solve all issues; not even for 64bit architectures when running this code in say 15 years.
There was a problem hiding this comment.
alright thx, I ll get back to it in couple of hours.
ext/session/session.c
Outdated
| #ifdef ZEND_ENABLE_ZVAL_LONG64 | ||
| const zend_long maxexpire = ((ZEND_LONG_MAX - INT_MAX) / 60) - 1; | ||
| #else | ||
| const zend_long maxexpire = ((ZEND_LONG_MAX / 2) / 60) - 1; | ||
| #endif |
There was a problem hiding this comment.
It seems to me that this is highly platform dependent, since the C standard makes no claim about time_t other than that it is an arithmetic type (could even be float/double). Current POSIX states "time_t shall be an integer type with a width of at least 64 bits". Now, that it not true for our x86 Windows builds (it's 32bit wide there); I don't know about other systems.
To handle this cleanly, we should probably determine something like TIME_MIN and TIME_MAX during configuration (and deal one way or another with non integral time_t implementations).
There was a problem hiding this comment.
Concretally, even QNX does not implement time_t as float/double.
for now checking time_t size at runtime just for POC purpose..
Setting with PHP_INT_MIN/PHP_INT_MAX lead to these.