Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ PHP NEWS
nested arrays). (Lazizbek Ergashev)
. Fixed bug GH-23115 (Stack overflow in compact() with deeply nested
arrays). (Lazizbek Ergashev)
. Fixed bug GH-23204 (Use-after-free in implode() when __toString() destroys
the array being joined). (Lazizbek Ergashev)

- Streams:
. Fixed bug GH-15836 (Use-after-free when a user stream filter accesses
Expand Down
4 changes: 4 additions & 0 deletions ext/standard/string.c
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,9 @@ PHPAPI void php_implode(const zend_string *glue, HashTable *pieces, zval *return

uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES(glue);

/* Converting an element may call __toString(), which can destroy pieces. */
GC_TRY_ADDREF(pieces);
Comment thread
lazerg marked this conversation as resolved.

ZEND_HASH_FOREACH_VAL(pieces, tmp) {
if (EXPECTED(Z_TYPE_P(tmp) == IS_STRING)) {
ptr->str = Z_STR_P(tmp);
Expand Down Expand Up @@ -1042,6 +1045,7 @@ PHPAPI void php_implode(const zend_string *glue, HashTable *pieces, zval *return
}

free_alloca(strings, use_heap);
GC_TRY_DTOR_NO_REF(pieces);
RETURN_NEW_STR(str);
}
/* }}} */
Expand Down
49 changes: 49 additions & 0 deletions ext/standard/tests/strings/gh23204.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
--TEST--
GH-23204 (Use-after-free in implode() when __toString() destroys the array)
--FILE--

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please credit the reporter within --CREDITS--.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, credited as iluuu1994.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not me, @e1abrador.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say the CREDITS section should no longer be used. Should the wording be extended with a "unless reporters have to be credited"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, sorry. Changed to e1abrador.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@arnaud-lb arnaud-lb Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed a change in #23215

<?php
class Unset_ implements Stringable {
public function __toString(): string {
global $a;
$a = null;
return "X";
}
}

$a = [new Unset_, 2, 3, 4];
echo "destroyed: ", implode(",", $a), "\n";
var_dump($a);

class Append implements Stringable {
public function __toString(): string {
global $b;
$b[] = str_repeat("y", 32);
return "X";
}
}

$b = [new Append, 2, 3, 4];
echo "appended: ", implode(",", $b), "\n";
echo "count: ", count($b), "\n";

class Boom implements Stringable {
public function __toString(): string {
global $c;
$c = null;
throw new Exception("boom");
}
}

$c = [new Boom, 2, 3, 4];
try {
implode(",", $c);
} catch (Exception $e) {
echo $e->getMessage(), "\n";
Comment thread
lazerg marked this conversation as resolved.
Outdated
}
?>
--EXPECT--
destroyed: X,2,3,4
NULL
appended: X,2,3,4
count: 5
boom
Comment thread
lazerg marked this conversation as resolved.
Outdated
Loading