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.
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.
My perspective is that memory issues in PHP that cause crashes are
generally not good and should be resolved wherever possible. While PHP 9
may address many of these issues, it is still far off and production code
migration will take even longer. Therefore, fixes now are still relevant.
That being said, I agree that fixes addressing odd/unlikely corner-cases
with a measurable performance impact are probably not-desired. My
suggestion is that this is handles on a case-by-case basis, balancing
performance impact to crash likelyhood.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
Hi
My perspective is that memory issues in PHP that cause crashes are
generally not good and should be resolved wherever possible. While PHP
9
may address many of these issues, it is still far off and production
code
migration will take even longer. Therefore, fixes now are still
relevant.
I agree with that. Also even if PHP 9 will remove all current
deprecations, there will certainly be new (engine) deprecations in the
PHP 9.x cycle and we're back to the status quo.
Best regards
Tim Düsterhus
Hi Gina
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.
snip
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.
I very much share this sentiment. Yes, ideally there are no ways to
trigger memory violations in PHP. However, while such bugs can be
catastrophic for other languages like JavaScript where it's expected to
run untrusted code on your computer, the same doesn't apply to PHP.
These bugs are effectively impossible to trigger by accident and thus
have no real-world consequence, and yet take up a very large chunk of
our time (both in terms of issues and reviews.
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).
I'd still very much be in favor of fixing these issues, mainly because
they are a big time sink for the security team as well, due to
false-positive reports. Arnaud and I were planning on proposing an RFC
that mitigates at least a large portion of them, before we got occupied
with other things.
I absolutely do not think each case should be considered individually,
because we already tried this and the issues never stop coming.
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.
The above might not all be problematic. E.g. the number of handlers that
produce output are fairly limited. The reports we do get that use them
rely on warnings printed to the output buffer, but if we can delay
warnings then this goes away as well. Similarly, not that many handlers
trigger autoloading. So this might be easier to control. But I did not
verify my claims here. But if cases remain that aren't possible to fix,
I wouldn't mind officially disallowing them.
Ilija
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).I'd still very much be in favor of fixing these issues, mainly because
they are a big time sink for the security team as well, due to
false-positive reports. Arnaud and I were planning on proposing an RFC
that mitigates at least a large portion of them, before we got occupied
with other things.
That is absolutely the ideal fix for this class of issues.
I absolutely do not think each case should be considered individually,
because we already tried this and the issues never stop coming.
True, but UB exists outside that class and those issues are worth the
attention imho.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws