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
```
(Taken from https://github.com/php/php-src/pull/23014)
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] https://github.com/php/php-src/issues/20018
