Skip to content

Commit

Permalink
Fix use-of-uninitialized-value with ??= on assert
Browse files Browse the repository at this point in the history
Normally, PHP evaluates all expressions in offsets (property or array), as well
as the right hand side of assignments before actually fetching the offsets. This
is well explained in this blog post.

https://www.npopov.com/2017/04/14/PHP-7-Virtual-machine.html#writes-and-memory-safety

For ??= we have a bit of a problem in that the rhs must only be evaluated if the
lhs is null or undefined. Thus, we have to first compile the lhs with BP_VAR_IS,
conditionally run the rhs and then re-fetch the lhs with BP_VAR_W to to make
sure the offsets are valid if they have been invalidated.

However, we don't want to just re-evaluate the entire lhs because it may contain
side-effects, as in $array[$x++] ??= 42;. In this case, we don't want to
re-evaluate $x++ because it would result in writing to a different offset than
was previously tested. The same goes for function calls, like
$array[foo()] ??= 42;, where the second call to foo() might result in a
different value. PHP behaves correctly in these cases. This is implemented by
memoizing sub-expressions in the lhs of ??= and reusing them when compiling the
lhs for the second time. This is done for any expression that isn't a variable,
i.e. anything that can (potentially) be written to.

Unfortunately, this also means that function calls are considered writable due
to their return-by-reference semantics, and will thus not be memoized. The
expression foo()['bar'] ??= 42; will invoke foo() twice. Even worse,
foo(bar()) ??= 42; will call both foo() and bar() twice, but
foo(bar() + 1) ??= 42; will only call foo() twice. This is likely not by design,
and was just overlooked in the implementation. The RFC does not specify how
function calls in the lhs of the coalesce assignment behaves. This should
probably be improved in the future.

Now, the problem this commit actually fixes is that ??= may memoize expressions
inside assert() function calls that may not actually execute. This is not only
an issue when using the VAR in the second expression (which would usually also
be skipped) but also when freeing the VAR. For this reason, it is not safe to
memoize assert() sub-expressions.

There are two possible solutions:

1. Don't memoize any sub-expressions of assert(), meaning they will execute
   twice.
2. Throw a compile error.

Option 2 is not quite simple, because we can't disallow all memoization inside
assert(), as that would break assertions like assert($array[foo()] ??= 'bar');.
Code like this is highly unlikely (and dubious) but possible. In this case, we
would need to make sure that a memoized value could not be used across the
assert boundary it was created in. The complexity for this is not worthwhile. So
we opt for option 1 and disable memoization immediately inside assert().

Fixes GH-11580
Closes GH-11581
  • Loading branch information
iluuu1994 committed Jul 6, 2023
1 parent f47dc25 commit 84a2e48
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 0 deletions.
1 change: 1 addition & 0 deletions NEWS
Expand Up @@ -11,6 +11,7 @@ PHP NEWS
- Core:
. Fixed oss-fuzz #60011 (Mis-compilation of by-reference nullsafe operator).
(ilutov)
. Fixed use-of-uninitialized-value with ??= on assert. (ilutov)

- Date:
. Fixed bug GH-11368 (Date modify returns invalid datetime). (Derick)
Expand Down
13 changes: 13 additions & 0 deletions Zend/tests/gh11580.phpt
@@ -0,0 +1,13 @@
--TEST--
GH-11580: assert() with ??= operator can lead to use-of-uninitialized-value
--INI--
zend.assertions=0
--FILE--
<?php
assert(y)[y] ??= y;
?>
--EXPECTF--
Fatal error: Uncaught Error: Undefined constant "y" in %s:%d
Stack trace:
#0 {main}
thrown in %s on line %d
6 changes: 6 additions & 0 deletions Zend/zend_compile.c
Expand Up @@ -4083,6 +4083,10 @@ static void zend_compile_assert(znode *result, zend_ast_list *args, zend_string
zend_op *opline;
uint32_t check_op_number = get_next_op_number();

/* Assert expression may not be memoized and reused as it may not actually be evaluated. */
int orig_memoize_mode = CG(memoize_mode);
CG(memoize_mode) = ZEND_MEMOIZE_NONE;

zend_emit_op(NULL, ZEND_ASSERT_CHECK, NULL, NULL);

if (fbc && fbc_is_finalized(fbc)) {
Expand Down Expand Up @@ -4116,6 +4120,8 @@ static void zend_compile_assert(znode *result, zend_ast_list *args, zend_string
opline = &CG(active_op_array)->opcodes[check_op_number];
opline->op2.opline_num = get_next_op_number();
SET_NODE(opline->result, result);

CG(memoize_mode) = orig_memoize_mode;
} else {
if (!fbc) {
zend_string_release_ex(name, 0);
Expand Down

0 comments on commit 84a2e48

Please sign in to comment.