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
The block optimizer pass allows the use of sources of the preceding
block if the block is a follower and not a target. This causes issues
when trying to remove FREE instructions: if the source is not in the
block of the FREE, then the FREE and source are still removed. Therefore
the other successor blocks, which must consume or FREE the temporary,
will still contain the FREE opline. This opline will now refer to a
temporary that doesn't exist anymore, which most of the time results in
a crash. For these kind of non-local scenarios, we'll let the SSA
based optimizations handle those cases.
Closes GH-11251.
In older versions of GCC (<=4.5) designated initializers would not accept member
names nested inside anonymous structures. Instead, we need to use a positional
member wrapped in {}.
Fixes GH-11063
Closes GH-11212
There are more places in zend_hash.c where the resize happened after some values on the HashTable struct were set.
I reordered them all, but writing a test for these would rely on the particular amount of bytes allocated at given points in time.
This patch preserves the scratch registers of the SysV x86-64 ABI by storing
them to the stack and restoring them later. We need to do this to prevent the
registers of the caller from being corrupted. The reason these get corrupted
is because the compiler is unaware of the Valgrind replacement function and
thus makes assumptions about the original function regarding registers which
are not true for the replacement function.
For implementation I used a GCC and Clang attribute. A more general
approach would be to use inline assembly but that's also less portable
and quite hacky. This attributes is supported since GCC 7.x, but the
target option is only supported since 11.x. For Clang the target option
does not matter.
Closes GH-10221.
There is a typo which causes the AND and OR range inference to infer a
wider range than necessary. Fix this typo. There are many ranges for
which the inference is too wide, I just picked one for AND and one for
OR that I found through symbolic execution.
In this example test, the previous range inferred for test_or was [-27..-1]
instead of [-20..-1].
And the previous range inferred for test_and was [-32..-25]
instead of [-28..-25].
Closes GH-11170.
It's actually not php-cli specific, nor SAPI specific.
We should delay the registration of the function into the function table
until after the compilation was successful, otherwise the function is
mistakingly registered and a NULL dereference will happen when trying to
call it.
I based my test of Nikita's test, so credits to him for the test:
https://github.com/php/php-src/pull/8933#issuecomment-1259881008
Closes GH-10989.
At least on 32-bit, the address computations overflow in running the
test on CI with UBSAN enabled. Fix it by reordering the arithmetic.
Since a part of the expression is already used in the code above the
computation, this should not negatively affect performance.
Closes GH-10936.
Fixes GH-8789.
Fixes GH-10015.
This is one small part of the underlying bug for GH-10737, as in my
attempts to reproduce the issue I constantly hit this crash easily.
(The fix for the other underlying issue for that bug will follow soon.)
It's possible that a signal arrives at a thread that never handled a PHP
request before. This causes the signal globals to dereference a NULL
pointer because the TSRM pointers for the thread aren't set up to point
to the thread resources yet.
PR GH-9766 previously fixed this for master by ignoring the signal if
the thread didn't handle a PHP request yet. While this fixes the crash
bug, I think the solution is suboptimal for 3 reasons:
1) The signal is ignored and a message is printed saying there is a bug.
However, this is not a bug at all. For example in Apache, the signal
set up happens on child process creation, and the thread resource
creation happens lazily when the first request is handled by the
thread. Hence, the fact that the thread resources aren't set up yet
is not actually buggy behaviour.
2) I believe since it was believed to be buggy behaviour, that fix was
only applied to master, so 8.1 & 8.2 keep on crashing.
3) We can do better than ignoring the signal. By just acting in the
same way as if the signals aren't active. This means we need to
take the same path as if the TSRM had already shut down.
Closes GH-10861.
Fixes GH-10801
Named arguments are not supported by the constant evaluation routine, in
the sense that they are ignored. This causes two issues:
- It causes a crash because not all oplines belonging to the call are
removed, which results in SEND_VA{L,R} which should've been removed.
- It causes semantic issues (demonstrated in the test case).
This case never worked anyway, leading to crashes or incorrect behaviour,
so just prevent CTE of calls with named parameters for now.
We can choose to support it later, but introducing support for this in
a stable branch seems too dangerous.
This patch does not change the removal of SEND_* opcodes in remove_call
because the crash bug can't be triggered anymore with this patch as
there are no named parameters anymore and no variadic CTE functions
exist.
Closes GH-10811.
Fixes GH-8646
See https://github.com/php/php-src/issues/8646 for thorough discussion.
Interned strings that hold class entries can get a corresponding slot in map_ptr for the CE cache.
map_ptr works like a bump allocator: there is a counter which increases to allocate the next slot in the map.
For class name strings in non-opcache we have:
- on startup: permanent + interned
- on request: interned
For class name strings in opcache we have:
- on startup: permanent + interned
- on request: either not interned at all, which we can ignore because they won't get a CE cache entry
or they were already permanent + interned
or we get a new permanent + interned string in the opcache persistence code
Notice that the map_ptr layout always has the permanent strings first, and the request strings after.
In non-opcache, a request string may get a slot in map_ptr, and that interned request string
gets destroyed at the end of the request. The corresponding map_ptr slot can thereafter never be used again.
This causes map_ptr to keep reallocating to larger and larger sizes.
We solve it as follows:
We can check whether we had any interned request strings, which only happens in non-opcache.
If we have any, we reset map_ptr to the last permanent string.
We can't lose any permanent strings because of map_ptr's layout.
Closes GH-10783.
zend_update_static_property_ex() returns a zend_result, but the return
value is stored here in a bool. A bool is unsigned on my system, so in
case zend_update_static_property_ex() returns FAILURE (== -1) this gets
converted to 1 instead. This is not a valid zend_result value. This
means that (transitive) callers could mistakingly think the function
succeeded while it did in fact not succeed. Fix it by changing the type
to zend_result.
Closes GH-10691.
