Uh oh!
There was an error while loading. Please reload this page.
ext/session: reject non-numeric session.upload_progress.freq - #22579
ext/session: reject non-numeric session.upload_progress.freq#22579jorgsowa wants to merge 3 commits into
Conversation
| int new_freq = ZEND_ATOL(ZSTR_VAL(new_value)); | ||
| const char *str = ZSTR_VAL(new_value); | ||
| size_t len = ZSTR_LEN(new_value); | ||
| bool is_percentage = len > 0 && str[len - 1] == '%'; |
There was a problem hiding this comment.
What happens with trailing spaces? Not sure how our INI parser deals with those.
There was a problem hiding this comment.
That's good input. Is there any ini helper for such values? I added a comparison table to the PR description and test cases to the branch for my suggested solution for trailing whitespaces.
OnUpdateRfc1867Freq used ZEND_ATOL(), which silently truncates trailing garbage (e.g. "5 apples" becomes 5), the same parsing bug recently fixed for session.cookie_lifetime. Switch to is_numeric_string_ex() to validate the numeric portion strictly, still allowing the optional trailing "%" for percentage mode.
Covers the "%"-suffixed form of session.upload_progress.freq, alongside the plain-integer case added in rfc1867_invalid_settings_3.phpt.
….freq Whitespace surrounding the whole value (leading, or trailing after an optional "%") is tolerated, but whitespace between the number and the "%" suffix (e.g. "5 %") is rejected rather than silently accepted. The stored ini value is also normalized to a whitespace-free canonical form so ini_get() no longer echoes back stray whitespace.
3e7229e to
3f78a89CompareGirgias
commented
Jul 6, 2026
I wonder if it doesn't make more sense to normalize all INI settings so they don't have trailing/leading whitespace. @arnaud-lb do you know if this would cause any issues? |
@Girgias I think that it may corrupt some deliberate values. It makes sense to strip leading/trailing whitespaces in ini files such as foo = bar because the whitespaces are invisible and may be introduced by error. But not in Edit: However, OnUpdateLong does it (via zend_ini_parse_quantity), so why not for numeric values at least. |
| if (is_percentage && numeric_len > 0 && php_session_is_ini_whitespace(str[numeric_len - 1])) { | ||
| php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be of type int"); | ||
| return FAILURE; | ||
| } | ||
| zend_long new_freq = 0; | ||
| uint8_t type = is_numeric_string_ex(str, numeric_len, &new_freq, NULL, false, NULL, NULL); | ||
| if (type != IS_LONG) { | ||
| php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be of type int"); | ||
| return FAILURE; | ||
| } |
There was a problem hiding this comment.
These error messages are wrong, as the INI is not an int, it's an int followed by a % sign.
OnUpdateRfc1867Freqsilently truncates trailing characters (e.g. "5 apples" becomes 5), the same parsing bug recently fixed for session.cookie_lifetime. Switch tois_numeric_string_ex()to validate the numeric portion strictly, still allowing the optional trailing "%" for percentage mode.5"5""5"5%"5%""5%"5%" 5%"(raw, kept spaces)"5%"(normalized)5%"5% "(raw)"5%"(normalized)5 %"5 %"accepted silently5 apples"5 apples"accepted silently5 apples%"5 apples%"accepted silently" "accepted silently200%-1