Repository navigation
Array assignment fails when the array has been resized in error handler #13754
Description
Description
The simple reproduction code as follows.
<?php
$array = range(0, 7);
class helper{
public $a1;
public $a2;
public $a3;
public $a4;
public $a5;
public $a6;
public $a7;
public $a8;
public $a9;
public $a10;
public $a11;
public $a12;
public $a13;
public $a14;
public function hello() {
echo "maple";
}
}
set_error_handler(function($err, $msg) {
global $array;
global $helper;
$array[] = 1; // force resize
$helper = new helper();
});
function crash() {
global $array;
global $helper;
$array[0] = $var; // undefined notice
$helper->hello();
$helper->$a1 = 1337;
}
crash();Resulted in this output:
crashed for segmentfault.
But I expected this output instead:
no crash.
This has been an issue for years (at least I learned about it 4 years ago). I wrote a paper for exploiting this as a security vulnerability.
PHP Version
PHP 8.3.3
Operating System
Ubuntu 20.04
Activity
iluuu1994 commentedon Mar 19, 2024
Hi @m4p1e. Thank you for your report. We are aware of various such issues, but they are unfortunately fundamentally hard to solve for all cases. My latest attempt is here: #12805 Luckily, such issues only happen in code that is seemingly explicitly crafted to cause it.
I haven't read your paper yet, but I will spend some time on it.
m4p1e commentedon Mar 19, 2024
Thank you for your prompt response. In my paper, I've stated, "This problem is not destined to be easily resolved." Why do I care about it? I've been deeply involved in a project concerning the PHP sandbox for running untrusted PHP code. This issue fundamentally breaks the sandbox, hence we have to disable the function
set_error_handler. Thanks again for letting me know that there is a possible solution. I will check it soon. I also provide a possible fix in my paper. It involves modifying the existing interpretation process of array assignement or fecthing to support multi-index in one opline. It's the most direct approach in my opinion.iluuu1994 commentedon Mar 19, 2024
Creating multidimensional DIM opcodes would solve this problem, but not others.
This issue is very similar.
V2is an indirect value to thebarproperty. IfASSIGN_DIMstill emits the "Undefined variable" warning before the offset is assigned, the error handler can still change the zval pointed to byV2. For example:Compile php-src with
--enable-address-sanitizerand then run withUSE_ZEND_ALLOC=0 php test.php.The error handler swaps the zval pointed to by
V2on-the-fly, after we have normally already established the indirect value to be an array. However, it now contains an unexpectednullvalue. There's a plethora of similar issues. Destructors are also very similar. (Edit: Actually, looking atASSIGN_DIM, the problem is probably that the array is freed before it is assigned to, because fetching of the "container" operand happens before the fetching of the assigned value, i.e. op_data).The approach from #12805 tries to delay the emission of warnings to a safe point in time instead. More specifically, this is when the current opcode no longer accepts indirect values. The PR is relatively straight-forward. However, it is unfortunately not compatible with the JIT, and I did not invest more time to try and fix that, because the JIT is extremely complicated and I already invested too much time on this theoretical issue. There are also other challenges, namely manually identifying exactly what warnings need to be delayed.
m4p1e commentedon Mar 20, 2024
You're right. I missed the cases in object assignment and fetch.
I was thinking of exposing all possible warnings as early as possible to make sure they don't affect following operations, which is exactly the opposite of your approach.
I agree. I have closely reviewed your PR, and it is great. In particular, I like the idea of delaying the warnings from sequential fetches until their results have been used.
The reason I'm reporting this issue is also because of JIT. With the rapid development of JIT technology in PHP today, this issue may become increasingly challenging to fix, and in turn, may hinder the progress of JIT technology in PHP. So, I think it should be fixed as soon as possible.
That shouldn't be very difficult to just fix the user error handler-related issues. But if the delayed error is introduced as a more general mechanism in PHP, its implementation needs more discussion.
iluuu1994 commentedon Mar 20, 2024
That's exactly right. All the problematic opcodes are listed here:
198b22a#diff-773bdb31563a0f907c75068675f6056b25f003e61f46928a31d9837ae107460dR8103-R8113
They can be combined in all kinds of ways, so I believe creating compound opcodes is not a viable option.
Feel free to pick this PR back up and try to add JIT support. I won't have time in the immediate future, but I'm happy to discuss/help out where possible.
iluuu1994 commentedon Apr 22, 2026
We have more descriptive issues for this problem now, i.e. #20018.