Fix shm corruption with coercion in options of unserialize() #20129
Conversation
| Shm corruption with coercion in options of unserialize() | ||
| --FILE-- | ||
| <?php | ||
| unserialize("{}", ["allowed_classes" => [0]]); |
There was a problem hiding this comment.
What happens if you have a non-stringable object as an option?
There was a problem hiding this comment.
In the sense that it will throw and exit out at the check after the loop
There was a problem hiding this comment.
Cool that's the thing I was wondering about.
ext/standard/var.c
Outdated
| convert_to_string(entry); | ||
| lcname = zend_string_tolower(Z_STR_P(entry)); | ||
| /* Note: cannot use zval_get_tmp_string() here as we need to keep it in class_hash for longer than this loop. */ | ||
| zend_string *str = zval_get_string(entry); |
There was a problem hiding this comment.
Any reason for not using the try version of this API?
There was a problem hiding this comment.
Possible, but not necessary, as we will exit at the exception check a bit later.
ext/standard/var.c
Outdated
| ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(classes), entry) { | ||
| convert_to_string(entry); | ||
| lcname = zend_string_tolower(Z_STR_P(entry)); | ||
| /* Note: cannot use zval_get_tmp_string() here as we need to keep it in class_hash for longer than this loop. */ |
There was a problem hiding this comment.
Is this comment accurate? zend_tmp_string_release() uses zend_string_release_ex() (i.e. not zend_string_free()), so it seems the allocation can still survive the loop. But it doesn't seem better, so maybe just remove the comment if my assuption is correct.
There was a problem hiding this comment.
Yeah my bad, you're right. I originally had a solution with the tmp string API (first commit). What I missed is that we actually take a strong reference via zend_string_tolower, so we can use that API. I'll revert my last commit and merge after CI passes
This reverts commit 332581e.
3 participants
No description provided.