Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 3 additions & 1 deletion Zend/Optimizer/zend_inference.c
Original file line number Diff line number Diff line change
Expand Up @@ -5299,7 +5299,9 @@ ZEND_API bool zend_may_throw_ex(const zend_op *opline, const zend_ssa_op *ssa_op
case ZEND_ISSET_ISEMPTY_DIM_OBJ:
return (t1 & MAY_BE_OBJECT) || (t2 & MAY_BE_THROWING_KEY);
case ZEND_FETCH_DIM_IS:
return (t1 & MAY_BE_OBJECT) || (t2 & MAY_BE_THROWING_KEY);
return (t1 & MAY_BE_OBJECT) || (t2 & MAY_BE_THROWING_KEY)
/* "Illegal string offset" warning for leading-numeric string offsets */
|| ((t1 & MAY_BE_STRING) && (t2 & MAY_BE_STRING));
case ZEND_CAST:
switch (opline->extended_value) {
case IS_LONG:
Expand Down
28 changes: 22 additions & 6 deletions ext/opcache/jit/zend_jit_helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -1112,17 +1112,35 @@ static void ZEND_FASTCALL zend_jit_fetch_dim_str_is_helper(zend_string *str, zva
switch (Z_TYPE_P(dim)) {
/* case IS_LONG: */
case IS_STRING:
if (IS_LONG == is_numeric_string(Z_STRVAL_P(dim), Z_STRLEN_P(dim), NULL, NULL, false)) {
break;
{
bool trailing_data = false;
/* Allow errors to warn on leading-numeric strings, like the VM */
if (IS_LONG != is_numeric_string_ex(Z_STRVAL_P(dim), Z_STRLEN_P(dim), &offset,
NULL, /* allow errors */ true, NULL, &trailing_data)) {
ZVAL_NULL(result);
return;
}
ZVAL_NULL(result);
return;
if (UNEXPECTED(trailing_data)) {
if (!(GC_FLAGS(str) & IS_STR_INTERNED)) {
GC_ADDREF(str);
}
zend_error(E_WARNING, "Illegal string offset \"%s\"", Z_STRVAL_P(dim));
if (!(GC_FLAGS(str) & IS_STR_INTERNED) && UNEXPECTED(GC_DELREF(str) == 0)) {
zend_string_efree(str);
ZVAL_NULL(result);
return;
}
}
break;
}
case IS_UNDEF:
zend_jit_undefined_op_helper(EG(current_execute_data)->opline->op2.var);

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 undefined-variable warning here can free str the same way: with global $s; return $s[$undef] ?? 1; and an error handler doing $GLOBALS['s'] = null, function JIT then reads the freed string (valgrind; PHP-8.4 has it without this PR too). Could the new guard cover this case as well, like the VM's IS_UNDEF branch does? The VM's own trailing-data warning in zend_fetch_dimension_address_read() has no guard either and shows the same invalid read without opcache.

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.

I created separate PR for this bug fix: #25329

It's valid bug, but I would like to keep all fixes separately. It's better for clearer git history.

ZEND_FALLTHROUGH;
case IS_DOUBLE:
case IS_NULL:
case IS_FALSE:
case IS_TRUE:
offset = zval_get_long_func(dim, /* is_strict */ false);
break;
case IS_REFERENCE:
dim = Z_REFVAL_P(dim);
Expand All @@ -1134,8 +1152,6 @@ static void ZEND_FASTCALL zend_jit_fetch_dim_str_is_helper(zend_string *str, zva
ZVAL_NULL(result);
return;
}

offset = zval_get_long_func(dim, /* is_strict */ false);
} else {
offset = Z_LVAL_P(dim);
}
Expand Down
39 changes: 39 additions & 0 deletions ext/opcache/tests/jit/fetch_dim_is_string_offset_001.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
--TEST--
JIT FETCH_DIM_IS: string offsets given as strings behave like the VM
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=function
opcache.file_update_protection=0
--EXTENSIONS--
opcache
--FILE--
<?php
function test(string $s, string $k) {
return $s[$k] ?? "default";
}

foreach (["1", "-1", "1 ", " 1", "1x", "-1x", "9x", "x", "", "1.5", "1.5x", "99999999999999999999"] as $k) {
echo var_export($k, true), ": ";
var_dump(test("abc", $k));
}
?>
--EXPECTF--
'1': string(1) "b"
'-1': string(1) "c"
'1 ': string(1) "b"
' 1': string(1) "b"
'1x':
Warning: Illegal string offset "1x" in %s on line %d
string(1) "b"
'-1x':
Warning: Illegal string offset "-1x" in %s on line %d
string(1) "c"
'9x':
Warning: Illegal string offset "9x" in %s on line %d
string(7) "default"
'x': string(7) "default"
'': string(7) "default"
'1.5': string(7) "default"
'1.5x': string(7) "default"
'99999999999999999999': string(7) "default"
35 changes: 35 additions & 0 deletions ext/opcache/tests/jit/fetch_dim_is_string_offset_002.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
--TEST--
JIT FETCH_DIM_IS: leading-numeric string offset conversion may throw
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=function
opcache.file_update_protection=0
--EXTENSIONS--
opcache
--FILE--
<?php
set_error_handler(function ($errno, $errstr) {
throw new Exception($errstr);
});

function make(int $n) {
return str_repeat("abc", $n);
}

function consume($v) {
echo "unreachable\n";
}

function test(string $s) {
consume($s["1x"] ?? "default");
}

try {
test(make(2));
} catch (Exception $e) {
echo $e::class, ": ", $e->getMessage(), "\n";
}
?>
--EXPECT--
Exception: Illegal string offset "1x"
35 changes: 35 additions & 0 deletions ext/opcache/tests/jit/fetch_dim_is_string_offset_003.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
--TEST--
JIT FETCH_DIM_IS: leading-numeric string offset may throw between call arguments
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=function
opcache.file_update_protection=0
--EXTENSIONS--
opcache
--FILE--
<?php
set_error_handler(function ($errno, $errstr) {
throw new Exception($errstr);
});

function make(int $n) {
return str_repeat("abc", $n);
}

function consume($a, $b) {
echo "unreachable\n";
}

function test(string $s, string $t) {
consume($t, isset($s["1x"][0]));
}

try {
test(make(2), make(3));
} catch (Exception $e) {
echo $e::class, ": ", $e->getMessage(), "\n";
}
?>
--EXPECT--
Exception: Illegal string offset "1x"
31 changes: 31 additions & 0 deletions ext/opcache/tests/jit/fetch_dim_is_string_offset_004.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
--TEST--
JIT FETCH_DIM_IS: leading-numeric string offset warning may release the string
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=function
opcache.file_update_protection=0
--EXTENSIONS--
opcache
--FILE--
<?php
set_error_handler(function ($errno, $errstr) {
echo "Warning: $errstr\n";
$GLOBALS['s'] = null;
});

function make(int $n) {
return str_repeat("abc", $n);
}

function test() {
global $s;
return $s["1x"] ?? "default";
}

$s = make(2);
var_dump(test());
?>
--EXPECT--
Warning: Illegal string offset "1x"
string(7) "default"
34 changes: 34 additions & 0 deletions ext/opcache/tests/opt/dce_023.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
--TEST--
DCE must not remove FETCH_DIM_IS of a nested isset()/empty() with a leading-numeric string offset
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.optimization_level=-1
opcache.file_update_protection=0
--EXTENSIONS--
opcache
--FILE--
<?php
set_error_handler(function ($errno, $errstr) {
throw new Exception($errstr);
});

function test_isset(string $s) {
$x = isset($s["1x"][0]);
}

function test_empty(string $s) {
$x = empty($s["1x"][0]);
}

foreach (['test_isset', 'test_empty'] as $f) {
try {
$f("abc");
} catch (Exception $e) {
echo $e::class, ": ", $e->getMessage(), "\n";
}
}
?>
--EXPECT--
Exception: Illegal string offset "1x"
Exception: Illegal string offset "1x"
Loading