ext/session: fix cookie_lifetime overflow#21704
Conversation
When session.cookie_lifetime was set to a value larger than maxcookie, OnUpdateCookieLifetime returned SUCCESS without updating the internal long value, causing ini_get() string and PS(cookie_lifetime) to go out of sync. Now the value is properly clamped to maxcookie with both the string and internal long updated consistently, and a warning is emitted.
Girgias
left a comment
There was a problem hiding this comment.
While at it could you fix the way we parse the string as well? As this probably allows non numeric strings and float strings.
Girgias
left a comment
There was a problem hiding this comment.
Getting there, minor comments. Thanks for tackling this :)
| } else if (lval > maxcookie) { | ||
| php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT ", value clamped to maximum", maxcookie); | ||
| zend_long *p = ZEND_INI_GET_ADDR(); | ||
| *p = maxcookie; | ||
| entry->value = zend_long_to_str(maxcookie); | ||
| return SUCCESS; | ||
| } |
There was a problem hiding this comment.
I think it was a bug before for it to return SUCCESS, so I would rather have it return FAILURE and warn then silently change behaviour.
Effectively just change the prior if condition to if (lval < 0 || lval > maxcookie) {
There was a problem hiding this comment.
Sure. It changes logic and may break some implementations, but it's fine to me.
| if (oflow != 0) { | ||
| php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT, maxcookie); | ||
| } else { | ||
| php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be an integer"); |
There was a problem hiding this comment.
The usual error message is something along the line of must be of type int.
There was a problem hiding this comment.
Applied + created an addition to CODING_CONVENTIONS.md in #21761
b99050e to
83991b6
Compare
When
session.cookie_lifetimewas set to a value larger than maxcookie,OnUpdateCookieLifetimereturned SUCCESS without updating the internal long value, causing ini_get() string and PS(cookie_lifetime) to go out of sync.Now the value is properly clamped to maxcookie with both the string and internal long updated consistently, and a warning is emitted.
Edit:
Added validation of value of maxcookie, only numeric strings are allowed.