-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix access on NULL when printing backtrace with freed generator #15952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this fix I would have managed to build myself 😁
I wasn't sure if the correct fix would be to fix zend_fetch_debug_backtrace()
to take a missing func
into account; or if the correct fix would be changing the engine so that this case cannot happen (expecially since the unknown()
is a little misleading / ugly).
I can confirm that this PR fixes the issue for my original use case as well.
Ok, I'll have a closer look. The main cause seems to be the fake frame inserted by yield from generators, and there's some code already present in zend_fetch_debug_backtrace() that should handle this frame. But it seems during close it doesn't correctly trigger anymore. I don't understand the details yet, I'll report back. |
Ok, this indeed looks like a better approach. I was also able to simplify the reproducer. diff --git a/Zend/tests/gh15851.phpt b/Zend/tests/gh15851.phpt
new file mode 100644
index 0000000000..bb333646b9
--- /dev/null
+++ b/Zend/tests/gh15851.phpt
@@ -0,0 +1,28 @@
+--TEST--
+GH-15851: Access on NULL when printing backtrace with freed generator
+--FILE--
+<?php
+
+class Foo {
+ public function __destruct() {
+ debug_print_backtrace();
+ }
+}
+
+function bar() {
+ yield from foo();
+}
+
+function foo() {
+ $foo = new Foo();
+ yield;
+}
+
+$gen = bar();
+foreach ($gen as $dummy);
+
+?>
+--EXPECTF--
+#0 %s(%d): Foo->__destruct()
+#1 %s(%d): foo()
+#2 %s(%d): bar()
diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h
index 3bc01a597f..86ce0d1867 100644
--- a/Zend/zend_vm_execute.h
+++ b/Zend/zend_vm_execute.h
@@ -4581,11 +4581,13 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_GENERATOR_RETURN_SPEC_CONST_HA
}
}
- EG(current_execute_data) = EX(prev_execute_data);
+ zend_execute_data *prev_execute_data = EX(prev_execute_data);
/* Close the generator to free up resources */
zend_generator_close(generator, 1);
+ EG(current_execute_data) = prev_execute_data;
+
/* Pass execution back to handling code */
ZEND_VM_RETURN();
} |
Set the func field for the empty top frame in cli to distinguish it from the fake generator frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right now.
This is the counterpart of phpGH-15952.
This is the counterpart of phpGH-15952.
Fixes GH-15851