ext/session: only return false when could not encode session at all#21181
ext/session: only return false when could not encode session at all#21181Girgias wants to merge 2 commits intophp:masterfrom
Conversation
ext/session/session.c
Outdated
| // TODO warn that ID cannot be verified? else { } | ||
| } | ||
| /* Read is required to make new session data at this point. */ | ||
| zend_string *data; |
There was a problem hiding this comment.
I think it should be initialised to NULL wdyt ?
There was a problem hiding this comment.
Hm, it wasn't previously though, so is that a bug then do you think?
There was a problem hiding this comment.
not sure all callbacks guarantee initialisation, this is all I m saying but I do not session that well :)
There was a problem hiding this comment.
I think it was always initialized prior to the rename because it would either be NULL or hold a dangling pointer. So not sure there is a bug, but I'll initialize it to NULL just because it's the most sensible.
| smart_str_appendc(&buf, PS_DELIMITER); | ||
| php_var_serialize(&buf, struc, &var_hash); | ||
| ); | ||
| PHP_VAR_SERIALIZE_DESTROY(var_hash); |
There was a problem hiding this comment.
This introduces a bug: now var_hash is destroyed twice because it is already destroyed at line 1039 too. I think you may drop line 1039.
ext/session/session.c
Outdated
| // TODO warn that ID cannot be verified? else { } | ||
| } | ||
| /* Read is required to make new session data at this point. */ | ||
| zend_string *data; |
There was a problem hiding this comment.
Hm, it wasn't previously though, so is that a bug then do you think?
This also fixes bug 71162
Girgias
force-pushed
the
2026-02-session-if-encode-null-then-failure
branch
from
51cc14b to
37698f3
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Looks logical, not sure how big the BC impact is, I'll leave that assessment to you. Although I presume that the edge case where this happens doesn't occur much in practice.
3 participants
This also fixes bug 71162.
I'm targeting master with this change as it is long-standing behaviour, but I don't really understand why an "empty session" would fail to encode. Similarly, with a "partial" encoding of it when discarding numeric keys.
The biggest impact is that
session_encode()now only returnsfalsein very limited cases and for empty sessions returns an empty string, which is how it was encoded anyway when actually performing session writes.