Fix memory leaks due to throw expressions

[nikic]

This introduces accurate tracking in the compiler of all the variables that are currently live. There's some overlap with the loop_var stack, and both can probably be combined (and preferably should be).

This allows us to free all pending variables before compiling a throw expression.

While the tracking here works, the actual freeing is incorrect as-is. The issue is that this just unconditionally frees everything, but of course the throw itself will also free the same variables based on live ranges, if the live ranges are present (without opcache).

I'm not quite sure on how to best handle this yet.

[nikic]

Here are some additional considerations on issues we need to deal with if we ever introduce a "block expression" concept, as part of match expressions or otherwise. While not needed right now, we should try to design this in a way that covers that case as well.

foreach ($x as $y) {
    $foo = new stdClass + {
        foreach ($a as $b) {
            break 2;
        }
        42
    };
}

Here the break 2 also needs to free new stdClass. In order to handle this, we will definitely need a unified "unwind stack", that includes temporaries, loop vars (and loop depth indication) and finally fastcall information.

try {
    $foo = new stdClass + {
        try {
            throw $x;
        } catch (Exception1) {}
        42
    };
} catch (Exception2) {}

Depending on which catch catches the exception, new stdClass may or may not need to be freed. This automatically works with our automatic live range based unwinder, but not if we want to manually emit free opcodes, in which case we'd also have to explicitly jump into the catch blocks, similar to what we do for return and finally.

[nikic]

Simple alternative that addresses just the throw expression issue in a minimal way: #5450

[iluuu1994]

Thanks for working on this!

if we ever introduce a "block expression" concept

Unfortunately we have this issue already in the match expression RFC.

function test() {
    var_dump([new stdClass(), match(true) {
        default => {
            return 'test';
            'not reached'
        }
    }]);
}

test();

Zend/zend_objects.c(196) : Freeing 0x0000000109471190 (40 bytes)

I think I will probably proceed without block expressions at this point (but with statement lists). Even if we could solve this in time people don't seem very happy with the proposed block syntax.

Either way, this probably needs to be solved at some point. It would make sense converting break, continue and goto to expressions at some point which I reckon would cause the same problems.

[nikic]

@iluuu1994 Yes, converting break/continue/goto to expressions would cause the same issue. We discussed this with @bwoebi on Friday, and our conclusion was basically that this approach would be right for handling of break/continue/return expressions (or them occurring in block expressions), but that this is not the right fix for the throw case. Exception handling should continue to always use live-range based automatic unwinding.

One of the reasons is that this issue is not limited to exceptions actually thrown by the throw. If live ranges end up being discarded, then leaks can also occur if an exception is thrown prior to the throw. Something like this maybe:

 new stdClass(throw new Exception(throwSomethingHere()))

Emitting explicit frees for the throw will not help us with the exception thrown from throwSomethingHere() (assuming live ranges got dropped).

So, if we want to solve this in a more general way than #5450, I think we really need to either prevent the live ranges from being dropped in opcache, or make opcache insert explicit frees when they get dropped.

[nikic]

Closing this one for now. The currently problematic case is fixed bug #5450. We can pick this work up again if and when we convert something like break into an expression.

[iluuu1994]

Thanks Nikita, makes sense. It doesn't look like the match expression will pass with blocks anyway.