Fix GH-23204: use-after-free when __toString() destroys an array argument - #23207
Fix GH-23204: use-after-free when __toString() destroys an array argument#23207lazerg wants to merge 4 commits into
Conversation
lazerg
changed the title
|
The Windows job failed on ext/pdo_odbc/tests/long_columns.phpt, which could not connect to its SQL Server instance. That is unrelated to this change. |
| @@ -0,0 +1,107 @@ | |||
| --TEST-- | |||
| GH-23204 (Use-after-free when __toString() destroys the array being read) | |||
| --FILE-- | |||
There was a problem hiding this comment.
Please credit the reporter within --CREDITS--.
There was a problem hiding this comment.
Added, credited as iluuu1994.
There was a problem hiding this comment.
The docs say the CREDITS section should no longer be used. Should the wording be extended with a "unless reporters have to be credited"?
There was a problem hiding this comment.
My mistake, sorry. Changed to e1abrador.
There was a problem hiding this comment.
The docs say the CREDITS section should no longer be used. Should the wording be extended with a "unless reporters have to be credited"?
The reasoning behind this is that git commits can track authors via Co-authored-by, which is complete bs. CREDITS section is a much more precise indication of authorship than a commit-wide author tag. So I purposefully ignore that docs and I believe that line should be removed.
There was a problem hiding this comment.
It looks like that this paragraph of the docs is only considering the case of the commit authors crediting themselves. It makes sense to me that no CREDITS section should be used in this case.
We could expand this paragraph with the case of committing a reproducer that was contributed by someone else.
devnexen
left a comment
devnexen
left a comment
There was a problem hiding this comment.
lgtm once remark is addressed
6 participants
implode() walks the array with ZEND_HASH_FOREACH_VAL while holding no reference on it. Converting a Stringable element runs user code, and if that code drops the last remaining reference to the array (
$a = null;from __toString()), arData is freed and the next iteration reads freed memory. strtr() and str_replace() read their array arguments the same way and crash the same way, so they are fixed here too.Taking a reference on the table for the duration of the read keeps it alive and turns an in-place mutation into a separation instead, same as zend_compare_symbol_tables() does around zend_hash_compare(). In implode() the reference is released after the pieces have been concatenated, since the collected zend_strings are still owned by the array until then.
Fixes #23204.