Skip to content

Commit

Permalink
Fix GH-16261: Reference invariant broken in mb_convert_variables()
Browse files Browse the repository at this point in the history
The behaviour is weird in the sense that the reference must get
unwrapped. What ended up happening is that when destroying the old
reference the sources list was not cleaned properly. We add handling for
that. Normally we would use use ZEND_TRY_ASSIGN_STRINGL but that doesn't
work here as it would keep the reference and change values through
references (see bug #26639).

Closes GH-16272.
  • Loading branch information
nielsdos committed Oct 7, 2024
1 parent 71222f7 commit bf70d9b
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 2 deletions.
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ PHP NEWS
. Fix GH-16136 (Memory leak in php_ldap_do_modify() when entry is not a
proper dictionary). (Girgias)

- MBString:
. Fixed bug GH-16261 (Reference invariant broken in mb_convert_variables()).
(nielsdos)

- OpenSSL:
. Fixed stub for openssl_csr_new. (Jakub Zelenka)

Expand Down
19 changes: 17 additions & 2 deletions ext/mbstring/mbstring.c
Original file line number Diff line number Diff line change
Expand Up @@ -3289,7 +3289,7 @@ static int mb_recursive_convert_variable(mbfl_buffer_converter *convd, zval *var
if (ret != NULL) {
zval_ptr_dtor(orig_var);
// TODO: avoid reallocation ???
ZVAL_STRINGL(orig_var, (char *)ret->val, ret->len);
ZVAL_STRINGL(orig_var, (const char *) ret->val, ret->len);
efree(ret->val);
}
} else if (Z_TYPE_P(var) == IS_ARRAY || Z_TYPE_P(var) == IS_OBJECT) {
Expand All @@ -3305,7 +3305,22 @@ static int mb_recursive_convert_variable(mbfl_buffer_converter *convd, zval *var

ht = HASH_OF(var);
if (ht != NULL) {
ZEND_HASH_FOREACH_VAL_IND(ht, entry) {
ZEND_HASH_FOREACH_VAL(ht, entry) {
/* Can be a typed property declaration, in which case we need to remove the reference from the source list.
* Just using ZEND_TRY_ASSIGN_STRINGL is not sufficient because that would not unwrap the reference
* and change values through references (see bug #26639). */
if (Z_TYPE_P(entry) == IS_INDIRECT) {
ZEND_ASSERT(Z_TYPE_P(var) == IS_OBJECT);

entry = Z_INDIRECT_P(entry);
if (Z_ISREF_P(entry) && Z_TYPE_P(Z_REFVAL_P(entry)) == IS_STRING) {
zend_property_info *info = zend_get_typed_property_info_for_slot(Z_OBJ_P(var), entry);
if (info) {
ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(entry), info);
}
}
}

if (mb_recursive_convert_variable(convd, entry)) {
if (Z_REFCOUNTED_P(var)) {
Z_UNPROTECT_RECURSION_P(var);
Expand Down
44 changes: 44 additions & 0 deletions ext/mbstring/tests/gh16261.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
GH-16261 (Reference invariant broken in mb_convert_variables())
--EXTENSIONS--
mbstring
--FILE--
<?php
class Test {
public string $x;
public string $y;
public array $z;
}
$test = new Test;
$ref = "hello";
$ref2 = "world";
$ref3 = [&$ref2];
$test->x =& $ref;
$test->z =& $ref3;
mb_convert_variables("EUC-JP", "Shift_JIS", $test);

class Test2 {
public function __construct(public string $x) {}
}
$test2 = new Test2("foo");

mb_convert_variables("EUC-JP", "Shift_JIS", $test->x);

var_dump($test, $test2);
?>
--EXPECT--
object(Test)#1 (2) {
["x"]=>
string(5) "hello"
["y"]=>
uninitialized(string)
["z"]=>
&array(1) {
[0]=>
string(5) "world"
}
}
object(Test2)#2 (1) {
["x"]=>
string(3) "foo"
}

0 comments on commit bf70d9b

Please sign in to comment.