Hello internals,
Over the last few years, more and more issues have been opened where a user defined error handler (or other callbacks) causes a Use After Free (UAF) when said error handler modifiers the variable/property that triggered the warning/deprecation/whatever.
These issues are usually found via fuzzing, or more recently LLMs.
One such example is:
class Victim {
public $tag = "alive";
public function target() {
echo $this->tag;
}
}
class Holder extends Victim {}
set_error_handler(function ($n, $s) {
if (str_contains($s, 'Callables of the form')) {
$GLOBALS['cb'] = null;
gc_collect_cycles();
}
return true;
});
$cb = [new Holder(), 'Victim::target'];
call_user_func($cb); // SIGSEGV
Nobody that wants to use PHP to do anything useful would write such an error handler, however mitigating these bugs requires tedious and intrusive fixes usually acting in the following way:
- Store the underlying zval value
- Increment the refcount of the stored value
- Emit the warning/deprecation/whatever
- If the zval was modified in some way, restore it (usually) by destroying the new value, or decrement the refcount
This reference tracking dance adds complexity to the engine, but also has negative performance implications for the vast majority of users not doing anything stupid in their error handlers (or other callbacks).
Especially as the most common triggering vector are engine deprecation or warnings that will be remove/promoted to throwing Errors in the future, where it will be very *easy* to miss removing this refcounting dance.
One potential idea that has been floating around is to delay error handlers. [1]
However, these issue are likely to also happen from other callbacks such as a class autoloader, a tick handler, an output handler, etc. as nothing prevents them from messing around with $GLOBALS or binding a variable to its scope.
While having C memory bugs is *far* from ideal, the fact that a majority of them will disappear with PHP 9 when deprecation are removed and warnings are promoted to Error makes this whackamole bug chase a waste of time. Especially as these issues have been present for *decades* and nobody except fuzzers and LLMs have run into these issues.
As such I'd like to hear the opinions of Internals on if we can come to a consensus (ideally without an RFC...) that registering callbacks that mess around with state in non-intended ways is Undefined Behaviour (UB).
While UB is far from desirable, it is not unprecedented in PHP.
Prior to PHP 8, the behaviour of internal functions when given values of invalid types was considered UB, sometimes it returned false, sometimes, null, sometimes it threw an Error.
This was fixed by throwing TypeErrors consistently.
Best regards,
Gina P. Banyard
PS: This is also why removing warnings from the engine by converting them to exceptions is highly desirable, as exceptions have none of these issues as the execution is stopped.
Error handlers specifically run in the middle of an operation which is how they can cause so much chaos. Tick handlers may also have these issues but this might actually be prevented by only calling them at sensible times after an operation has been fully performed.
[1] Delaying error handlers · Issue #20018 · php/php-src · GitHub