Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Sep 18, 2024

Fixes GH-15851

Copy link
Member

@TimWolla TimWolla left a 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.

@iluuu1994
Copy link
Member Author

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.

@iluuu1994
Copy link
Member Author

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.
Copy link
Member

@bwoebi bwoebi left a 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.

@iluuu1994 iluuu1994 closed this in 706bcdb Sep 27, 2024
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Oct 1, 2024
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Oct 1, 2024
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Oct 21, 2024
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Oct 21, 2024
iluuu1994 added a commit that referenced this pull request Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants