Skip to content

Commit 09547c6

Browse files
dstogovdstogov
committed
Fix clobering of operand by error handler in assignment to string offset
In some cases new code requires two reallocations insead of one. Fixes oss-fuzz #31716, #36196, #39739 and #40002
1 parent 9f6ab78 commit 09547c6

File tree

6 files changed

+176
-81
lines changed

6 files changed

+176
-81
lines changed

‎Zend/tests/str_offset_006.phpt

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
--TEST--
2+
string offset 006 indirect string modification by error handler
3+
--FILE--
4+
<?php
5+
set_error_handler(function($code, $msg) {
6+
echo "Err: $msg\n";
7+
$GLOBALS['a']=null;
8+
});
9+
$a[$y]=$a.=($y);
10+
var_dump($a);
11+
?>
12+
--EXPECT--
13+
Err: Undefined variable $y
14+
Err: Undefined variable $y
15+
Err: String offset cast occurred
16+
NULL

‎Zend/tests/str_offset_007.phpt

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
--TEST--
2+
string offset 007 indirect string modification by error handler
3+
--FILE--
4+
<?php
5+
set_error_handler(function($code, $msg) {
6+
echo "Err: $msg\n";
7+
$GLOBALS['a']='';
8+
});
9+
$a=['a'];
10+
$a[0][$d]='b';
11+
var_dump($a);
12+
?>
13+
--EXPECT--
14+
Err: Undefined variable $d
15+
Err: String offset cast occurred
16+
string(0) ""

‎Zend/zend_execute.c

Lines changed: 105 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1368,44 +1368,40 @@ static zend_never_inline zend_long zend_check_string_offset(zval *dim, int type
13681368
zend_long offset;
13691369

13701370
try_again:
1371-
if (UNEXPECTED(Z_TYPE_P(dim) != IS_LONG)) {
1372-
switch(Z_TYPE_P(dim)) {
1373-
case IS_STRING:
1374-
{
1375-
bool trailing_data = false;
1376-
/* For BC reasons we allow errors so that we can warn on leading numeric string */
1377-
if (IS_LONG == is_numeric_string_ex(Z_STRVAL_P(dim), Z_STRLEN_P(dim), &offset, NULL,
1378-
/* allow errors */ true, NULL, &trailing_data)) {
1379-
if (UNEXPECTED(trailing_data) && type != BP_VAR_UNSET) {
1380-
zend_error(E_WARNING, "Illegal string offset \"%s\"", Z_STRVAL_P(dim));
1381-
}
1382-
return offset;
1371+
switch(Z_TYPE_P(dim)) {
1372+
case IS_LONG:
1373+
return Z_LVAL_P(dim);
1374+
case IS_STRING:
1375+
{
1376+
bool trailing_data = false;
1377+
/* For BC reasons we allow errors so that we can warn on leading numeric string */
1378+
if (IS_LONG == is_numeric_string_ex(Z_STRVAL_P(dim), Z_STRLEN_P(dim), &offset, NULL,
1379+
/* allow errors */ true, NULL, &trailing_data)) {
1380+
if (UNEXPECTED(trailing_data) && type != BP_VAR_UNSET) {
1381+
zend_error(E_WARNING, "Illegal string offset \"%s\"", Z_STRVAL_P(dim));
13831382
}
1384-
zend_illegal_string_offset(dim);
1385-
return 0;
1383+
return offset;
13861384
}
1387-
case IS_UNDEF:
1388-
ZVAL_UNDEFINED_OP2();
1389-
case IS_DOUBLE:
1390-
case IS_NULL:
1391-
case IS_FALSE:
1392-
case IS_TRUE:
1393-
zend_error(E_WARNING, "String offset cast occurred");
1394-
break;
1395-
case IS_REFERENCE:
1396-
dim = Z_REFVAL_P(dim);
1397-
goto try_again;
1398-
default:
1399-
zend_illegal_string_offset(dim);
1400-
return 0;
1385+
zend_illegal_string_offset(dim);
1386+
return 0;
14011387
}
1402-
1403-
offset = zval_get_long_func(dim);
1404-
} else {
1405-
offset = Z_LVAL_P(dim);
1388+
case IS_UNDEF:
1389+
ZVAL_UNDEFINED_OP2();
1390+
case IS_DOUBLE:
1391+
case IS_NULL:
1392+
case IS_FALSE:
1393+
case IS_TRUE:
1394+
zend_error(E_WARNING, "String offset cast occurred");
1395+
break;
1396+
case IS_REFERENCE:
1397+
dim = Z_REFVAL_P(dim);
1398+
goto try_again;
1399+
default:
1400+
zend_illegal_string_offset(dim);
1401+
return 0;
14061402
}
14071403

1408-
return offset;
1404+
return zval_get_long_func(dim);
14091405
}
14101406

14111407
static zend_never_inline ZEND_COLD void zend_wrong_string_offset(EXECUTE_DATA_D)
@@ -1535,17 +1531,43 @@ static zend_never_inline void zend_assign_to_string_offset(zval *str, zval *dim,
15351531
zend_uchar c;
15361532
size_t string_len;
15371533
zend_long offset;
1534+
zend_string *s;
15381535

1539-
offset = zend_check_string_offset(dim, BP_VAR_W EXECUTE_DATA_CC);
1540-
/* Illegal offset assignment */
1541-
if (UNEXPECTED(EG(exception) != NULL)) {
1542-
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
1543-
ZVAL_UNDEF(EX_VAR(opline->result.var));
1536+
/* separate string */
1537+
if (Z_REFCOUNTED_P(str) && Z_REFCOUNT_P(str) == 1) {
1538+
s = Z_STR_P(str);
1539+
} else {
1540+
s = zend_string_init(Z_STRVAL_P(str), Z_STRLEN_P(str), 0);
1541+
ZSTR_H(s) = ZSTR_H(Z_STR_P(str));
1542+
ZVAL_NEW_STR(str, s);
1543+
}
1544+
1545+
if (EXPECTED(Z_TYPE_P(dim) == IS_LONG)) {
1546+
offset = Z_LVAL_P(dim);
1547+
} else {
1548+
/* The string may be destroyed while throwing the notice.
1549+
* Temporarily increase the refcount to detect this situation. */
1550+
if (!(GC_FLAGS(s) & IS_ARRAY_IMMUTABLE)) {
1551+
GC_ADDREF(s);
1552+
}
1553+
offset = zend_check_string_offset(dim, BP_VAR_W EXECUTE_DATA_CC);
1554+
if (!(GC_FLAGS(s) & IS_ARRAY_IMMUTABLE) && GC_DELREF(s) == 0) {
1555+
zend_string_efree(s);
1556+
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
1557+
ZVAL_NULL(EX_VAR(opline->result.var));
1558+
}
1559+
return;
1560+
}
1561+
/* Illegal offset assignment */
1562+
if (UNEXPECTED(EG(exception) != NULL)) {
1563+
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
1564+
ZVAL_UNDEF(EX_VAR(opline->result.var));
1565+
}
1566+
return;
15441567
}
1545-
return;
15461568
}
15471569

1548-
if (offset < -(zend_long)Z_STRLEN_P(str)) {
1570+
if (UNEXPECTED(offset < -(zend_long)ZSTR_LEN(s))) {
15491571
/* Error on negative offset */
15501572
zend_error(E_WARNING, "Illegal string offset " ZEND_LONG_FMT, offset);
15511573
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
@@ -1554,9 +1576,28 @@ static zend_never_inline void zend_assign_to_string_offset(zval *str, zval *dim,
15541576
return;
15551577
}
15561578

1557-
if (Z_TYPE_P(value) != IS_STRING) {
1579+
if (offset < 0) { /* Handle negative offset */
1580+
offset += (zend_long)ZSTR_LEN(s);
1581+
}
1582+
1583+
if (UNEXPECTED(Z_TYPE_P(value) != IS_STRING)) {
1584+
/* The string may be destroyed while throwing the notice.
1585+
* Temporarily increase the refcount to detect this situation. */
1586+
if (!(GC_FLAGS(s) & IS_ARRAY_IMMUTABLE)) {
1587+
GC_ADDREF(s);
1588+
}
1589+
if (UNEXPECTED(Z_TYPE_P(value) == IS_UNDEF)) {
1590+
zval_undefined_cv((opline+1)->op1.var EXECUTE_DATA_CC);
1591+
}
15581592
/* Convert to string, just the time to pick the 1st byte */
15591593
zend_string *tmp = zval_try_get_string_func(value);
1594+
if (!(GC_FLAGS(s) & IS_ARRAY_IMMUTABLE) && GC_DELREF(s) == 0) {
1595+
zend_string_efree(s);
1596+
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
1597+
ZVAL_NULL(EX_VAR(opline->result.var));
1598+
}
1599+
return;
1600+
}
15601601
if (UNEXPECTED(!tmp)) {
15611602
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
15621603
ZVAL_UNDEF(EX_VAR(opline->result.var));
@@ -1572,7 +1613,7 @@ static zend_never_inline void zend_assign_to_string_offset(zval *str, zval *dim,
15721613
c = (zend_uchar)Z_STRVAL_P(value)[0];
15731614
}
15741615

1575-
if (string_len != 1) {
1616+
if (UNEXPECTED(string_len != 1)) {
15761617
if (string_len == 0) {
15771618
/* Error on empty input string */
15781619
zend_throw_error(NULL, "Cannot assign an empty string to a string offset");
@@ -1582,24 +1623,34 @@ static zend_never_inline void zend_assign_to_string_offset(zval *str, zval *dim,
15821623
return;
15831624
}
15841625

1626+
/* The string may be destroyed while throwing the notice.
1627+
* Temporarily increase the refcount to detect this situation. */
1628+
if (!(GC_FLAGS(s) & IS_ARRAY_IMMUTABLE)) {
1629+
GC_ADDREF(s);
1630+
}
15851631
zend_error(E_WARNING, "Only the first byte will be assigned to the string offset");
1632+
if (!(GC_FLAGS(s) & IS_ARRAY_IMMUTABLE) && GC_DELREF(s) == 0) {
1633+
zend_string_efree(s);
1634+
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
1635+
ZVAL_NULL(EX_VAR(opline->result.var));
1636+
}
1637+
return;
1638+
}
1639+
/* Illegal offset assignment */
1640+
if (UNEXPECTED(EG(exception) != NULL)) {
1641+
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
1642+
ZVAL_UNDEF(EX_VAR(opline->result.var));
1643+
}
1644+
return;
1645+
}
15861646
}
15871647

1588-
if (offset < 0) { /* Handle negative offset */
1589-
offset += (zend_long)Z_STRLEN_P(str);
1590-
}
1591-
1592-
if ((size_t)offset >= Z_STRLEN_P(str)) {
1648+
if ((size_t)offset >= ZSTR_LEN(s)) {
15931649
/* Extend string if needed */
1594-
zend_long old_len = Z_STRLEN_P(str);
1595-
ZVAL_NEW_STR(str, zend_string_extend(Z_STR_P(str), (size_t)offset + 1, 0));
1650+
zend_long old_len = ZSTR_LEN(s);
1651+
ZVAL_NEW_STR(str, zend_string_extend(s, (size_t)offset + 1, 0));
15961652
memset(Z_STRVAL_P(str) + old_len, ' ', offset - old_len);
15971653
Z_STRVAL_P(str)[offset+1] = 0;
1598-
} else if (!Z_REFCOUNTED_P(str)) {
1599-
ZVAL_NEW_STR(str, zend_string_init(Z_STRVAL_P(str), Z_STRLEN_P(str), 0));
1600-
} else if (Z_REFCOUNT_P(str) > 1) {
1601-
Z_DELREF_P(str);
1602-
ZVAL_NEW_STR(str, zend_string_init(Z_STRVAL_P(str), Z_STRLEN_P(str), 0));
16031654
} else {
16041655
zend_string_forget_hash_val(Z_STR_P(str));
16051656
}

‎Zend/zend_vm_def.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2590,8 +2590,8 @@ ZEND_VM_C_LABEL(try_assign_dim_array):
25902590
FREE_OP_DATA();
25912591
UNDEF_RESULT();
25922592
} else {
2593-
dim = GET_OP2_ZVAL_PTR(BP_VAR_R);
2594-
value = GET_OP_DATA_ZVAL_PTR_DEREF(BP_VAR_R);
2593+
dim = GET_OP2_ZVAL_PTR_UNDEF(BP_VAR_R);
2594+
value = GET_OP_DATA_ZVAL_PTR_UNDEF(BP_VAR_R);
25952595
zend_assign_to_string_offset(object_ptr, dim, value OPLINE_CC EXECUTE_DATA_CC);
25962596
FREE_OP_DATA();
25972597
}

0 commit comments

Comments
 (0)