Fix use after free during shutdown destruction

[danog]

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

[danog]

Ping @nielsdos

[nielsdos]

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);
 					}
 				}
 			}

[dstogov]

We skipped zend_object_std_dtor() calls because we didn't need to deallocate the memory for each property separately. We relayed on Zend MM to deallocate all the request memory at once.

The problem was introduced by weak references that added notification in free_obj calback.
I'm not sure if this was a right/best place.
I think, a better quick fix would be checking IS_OBJ_WEAKLY_REFERENCED flag.

[danog]

Thank you @nielsdos and @dstogov, I went with Dmitry's fix as it seems less expensive compared to a double comparison against both zend_object_std_dtor and zend_weakmap_free_obj.

[nielsdos]

I would prefer that you add parentheses like I did in my example patch

[danog]

Done!

[nielsdos]

I will have a look and test on thursday evening as I'm not available until then.

[danog]

Thank you!