Skip to content

Fix use after free during shutdown destruction #18834

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

Open
wants to merge 3 commits into
base: PHP-8.3
Choose a base branch
from

Conversation

danog
Copy link
Contributor

@danog danog commented Jun 11, 2025

Fixes #18833 by removing the faulty fast path, which breaks weak reference notifications, also causing issues like phpredis/phpredis#2630.

@danog
Copy link
Contributor Author

danog commented Jun 11, 2025

Ping @nielsdos

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, nice find!
I think the solution needs changes however.
Rather than removing the fast shutdown path, it should also skip the weakmap free_obj handler. Note that we already skip the std free_obj handler.
There's 2 easy ways of doing this:

  1. Either extend the existing if condition to check for the weakmap free_obj handler.
  2. Or you add a check inside the function implementing the weakmap free_obj handler. You'd need to know whether we're in fast shutdown then though.

Option 1 is easier, option 2 might be cheaper. I'm not sure.

EDIT: Ah but I suppose that in option1, other internal classes holding onto weak refs can then trigger the same issue. Perhaps we should do it the other way around: call free_obj anyway if IS_OBJ_WEAKLY_REFERENCE is set...
Something like this perhaps:

diff --git a/Zend/zend_objects_API.c b/Zend/zend_objects_API.c
index 80f5b747db7..df4b961a903 100644
--- a/Zend/zend_objects_API.c
+++ b/Zend/zend_objects_API.c
@@ -104,7 +104,7 @@ ZEND_API void ZEND_FASTCALL zend_objects_store_free_object_storage(zend_objects_
 			if (IS_OBJ_VALID(obj)) {
 				if (!(OBJ_FLAGS(obj) & IS_OBJ_FREE_CALLED)) {
 					GC_ADD_FLAGS(obj, IS_OBJ_FREE_CALLED);
-					if (obj->handlers->free_obj != zend_object_std_dtor) {
+					if (obj->handlers->free_obj != zend_object_std_dtor || (GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) {
 						GC_ADDREF(obj);
 						obj->handlers->free_obj(obj);
 					}

or even:

diff --git a/Zend/zend_objects_API.c b/Zend/zend_objects_API.c
index 80f5b747db7..8ffdfa18af8 100644
--- a/Zend/zend_objects_API.c
+++ b/Zend/zend_objects_API.c
@@ -24,6 +24,7 @@
 #include "zend_API.h"
 #include "zend_objects_API.h"
 #include "zend_fibers.h"
+#include "zend_weakrefs.h"
 
 ZEND_API void ZEND_FASTCALL zend_objects_store_init(zend_objects_store *objects, uint32_t init_size)
 {
@@ -107,6 +108,8 @@ ZEND_API void ZEND_FASTCALL zend_objects_store_free_object_storage(zend_objects_
 					if (obj->handlers->free_obj != zend_object_std_dtor) {
 						GC_ADDREF(obj);
 						obj->handlers->free_obj(obj);
+					} else if (UNEXPECTED(GC_FLAGS(obj) & IS_OBJ_WEAKLY_REFERENCED)) {
+						zend_weakrefs_notify(obj);
 					}
 				}
 			}

@nielsdos nielsdos linked an issue Jun 11, 2025 that may be closed by this pull request
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.

Use after free with weakmaps dependent on destruction order
2 participants