From 9a8a1c9da129a72c9bf8f3392f954a9e5ca317ff Mon Sep 17 00:00:00 2001 From: Jim Zubov Date: Mon, 6 Feb 2017 18:05:34 -0500 Subject: [PATCH 1/5] Bug Fix: Corrupted class entries on shutdown when a destructor spawns another object (C) 2017 CommerceByte Consulting When zend_objects_store_call_destructors() is called from the shutdown sequence - it's calling the dtor's for remaining objects one by one in sequence of object handles. If the dtor spawns one or more objects, and the new objects happen to reuse the old handles - their dtor's are not called in this cycle. The dtor's are called later on, when zend_deactivete() kicks in, and the static property lists in the class entries are freed. This causes "Undefined static property" errors, and/or SIGSEGV. Solution: zend_object_store.no_reuse field is added Set to 0 on initialization, set to 1 on the shutdown sequence. zend_objects_store_put(zend_object *) checks the no_reuse flag, and never reuses the old handle slots if set. This way, the dtor's for newly spawned objects are guaranteed to be called in the zend_objects_store_call_destructors() loop. --- Zend/zend_objects_API.c | 4 +++- Zend/zend_objects_API.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Zend/zend_objects_API.c b/Zend/zend_objects_API.c index 858113aabd9ae..a0de331766cd3 100644 --- a/Zend/zend_objects_API.c +++ b/Zend/zend_objects_API.c @@ -32,6 +32,7 @@ ZEND_API void zend_objects_store_init(zend_objects_store *objects, uint32_t init objects->top = 1; /* Skip 0 so that handles are true */ objects->size = init_size; objects->free_list_head = -1; + objects->no_reuse = 0; memset(&objects->object_buckets[0], 0, sizeof(zend_object*)); } @@ -43,6 +44,7 @@ ZEND_API void zend_objects_store_destroy(zend_objects_store *objects) ZEND_API void zend_objects_store_call_destructors(zend_objects_store *objects) { + objects->no_reuse = 1; /* new objects spawned by dtors will never reuse unused slots, so their own dtors will be called further down the loop */ if (objects->top > 1) { uint32_t i; for (i = 1; i < objects->top; i++) { @@ -111,7 +113,7 @@ ZEND_API void zend_objects_store_put(zend_object *object) { int handle; - if (EG(objects_store).free_list_head != -1) { + if (!EG(objects_store).no_reuse && EG(objects_store).free_list_head != -1) { handle = EG(objects_store).free_list_head; EG(objects_store).free_list_head = GET_OBJ_BUCKET_NUMBER(EG(objects_store).object_buckets[handle]); } else { diff --git a/Zend/zend_objects_API.h b/Zend/zend_objects_API.h index 223060035e287..ebf048751900f 100644 --- a/Zend/zend_objects_API.h +++ b/Zend/zend_objects_API.h @@ -45,6 +45,7 @@ typedef struct _zend_objects_store { uint32_t top; uint32_t size; int free_list_head; + char no_reuse; /* to be set to true when shutting down, to avoid missing dtor call on objects spawned by another dtor */ } zend_objects_store; /* Global store handling functions */ From d94b067504dd949a52fb777fe0b505ab522a8682 Mon Sep 17 00:00:00 2001 From: Jim Zubov Date: Tue, 7 Feb 2017 09:19:16 -0500 Subject: [PATCH 2/5] The test scripts bug64720.phpt and bug68652.phpt were relying on the buggy behavior, when PHP returns "Undefined static property" error due to class entry corruption. With my fix for bug 74053, both tests return no errors now, I corrected the EXPECTF accordingly [Anybody please advice if I'm wrong?] Also created bug74053.phpt, for the code I mentioned in the bug description --- Zend/tests/bug64720.phpt | 5 ----- Zend/tests/bug68652.phpt | 7 ------- Zend/tests/bug74053.phpt | 43 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 12 deletions(-) create mode 100644 Zend/tests/bug74053.phpt diff --git a/Zend/tests/bug64720.phpt b/Zend/tests/bug64720.phpt index 45dee3e8c46b4..35b01e6a16290 100644 --- a/Zend/tests/bug64720.phpt +++ b/Zend/tests/bug64720.phpt @@ -45,8 +45,3 @@ $bar = new Bar(); $bar->test(); ?> --EXPECTF-- -Fatal error: Uncaught Error: Access to undeclared static property: Stat::$requests in %sbug64720.php:12 -Stack trace: -#0 [internal function]: Stat->__destruct() -#1 {main} - thrown in %sbug64720.php on line 12 diff --git a/Zend/tests/bug68652.phpt b/Zend/tests/bug68652.phpt index 8e54af2e34858..f0c9d5e9aa1f0 100644 --- a/Zend/tests/bug68652.phpt +++ b/Zend/tests/bug68652.phpt @@ -37,10 +37,3 @@ class Bar { $foo = new Foo(); ?> --EXPECTF-- -Fatal error: Uncaught Error: Access to undeclared static property: Bar::$instance in %sbug68652.php:%d -Stack trace: -#0 %s(%d): Bar::getInstance() -#1 [internal function]: Foo->__destruct() -#2 {main} - thrown in %sbug68652.php on line %d - diff --git a/Zend/tests/bug74053.phpt b/Zend/tests/bug74053.phpt new file mode 100644 index 0000000000000..e8fc02d387b5e --- /dev/null +++ b/Zend/tests/bug74053.phpt @@ -0,0 +1,43 @@ +--TEST-- +Bug #74053 (Corrupted class entries on shutdown when a destructor spawns another object) +--FILE-- + +--EXPECTF-- +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct +a::destruct +b::destruct From a6acffe421c1ea5e0d654169a17103285711596e Mon Sep 17 00:00:00 2001 From: Jim Zubov Date: Tue, 7 Feb 2017 10:51:09 -0500 Subject: [PATCH 3/5] Alignment fix, as per @nikic --- Zend/zend_objects_API.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Zend/zend_objects_API.h b/Zend/zend_objects_API.h index ebf048751900f..08b0a13a305b1 100644 --- a/Zend/zend_objects_API.h +++ b/Zend/zend_objects_API.h @@ -45,7 +45,7 @@ typedef struct _zend_objects_store { uint32_t top; uint32_t size; int free_list_head; - char no_reuse; /* to be set to true when shutting down, to avoid missing dtor call on objects spawned by another dtor */ + int no_reuse; /* to be set to true when shutting down, to avoid missing dtor call on objects spawned by another dtor */ } zend_objects_store; /* Global store handling functions */ From f9afc6aabc8af2da5874b32e11f9e4cca722c079 Mon Sep 17 00:00:00 2001 From: Jim Zubov Date: Tue, 7 Feb 2017 21:13:39 -0500 Subject: [PATCH 4/5] newly added zend_object_store.no_reuse is redefined as a global zend_object_store_no_reuse, to avoid alignment issues --- Zend/zend_objects_API.c | 8 +++++--- Zend/zend_objects_API.h | 1 - 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Zend/zend_objects_API.c b/Zend/zend_objects_API.c index a0de331766cd3..b64d413fc233f 100644 --- a/Zend/zend_objects_API.c +++ b/Zend/zend_objects_API.c @@ -26,13 +26,15 @@ #include "zend_API.h" #include "zend_objects_API.h" +ZEND_TLS zend_objects_store_no_reuse; /* Would make more sense to make it a member of zend_objects_store, defining as a global to not break alignments */ + ZEND_API void zend_objects_store_init(zend_objects_store *objects, uint32_t init_size) { objects->object_buckets = (zend_object **) emalloc(init_size * sizeof(zend_object*)); objects->top = 1; /* Skip 0 so that handles are true */ objects->size = init_size; objects->free_list_head = -1; - objects->no_reuse = 0; + zend_objects_store_no_reuse = 0; memset(&objects->object_buckets[0], 0, sizeof(zend_object*)); } @@ -44,7 +46,7 @@ ZEND_API void zend_objects_store_destroy(zend_objects_store *objects) ZEND_API void zend_objects_store_call_destructors(zend_objects_store *objects) { - objects->no_reuse = 1; /* new objects spawned by dtors will never reuse unused slots, so their own dtors will be called further down the loop */ + zend_objects_store_no_reuse = 1; /* new objects spawned by dtors will never reuse unused slots, so their own dtors will be called further down the loop */ if (objects->top > 1) { uint32_t i; for (i = 1; i < objects->top; i++) { @@ -113,7 +115,7 @@ ZEND_API void zend_objects_store_put(zend_object *object) { int handle; - if (!EG(objects_store).no_reuse && EG(objects_store).free_list_head != -1) { + if (!zend_objects_store_no_reuse && EG(objects_store).free_list_head != -1) { handle = EG(objects_store).free_list_head; EG(objects_store).free_list_head = GET_OBJ_BUCKET_NUMBER(EG(objects_store).object_buckets[handle]); } else { diff --git a/Zend/zend_objects_API.h b/Zend/zend_objects_API.h index 08b0a13a305b1..223060035e287 100644 --- a/Zend/zend_objects_API.h +++ b/Zend/zend_objects_API.h @@ -45,7 +45,6 @@ typedef struct _zend_objects_store { uint32_t top; uint32_t size; int free_list_head; - int no_reuse; /* to be set to true when shutting down, to avoid missing dtor call on objects spawned by another dtor */ } zend_objects_store; /* Global store handling functions */ From 1b1399c95d1358b23f234c14e84dda9b5007904e Mon Sep 17 00:00:00 2001 From: Jim Zubov Date: Thu, 9 Feb 2017 12:40:33 -0500 Subject: [PATCH 5/5] Added EG(flags) - executor global flags EG_FLAGS_IN_SHUTDOWN - is set when PHP is in shutdown state --- Zend/zend.c | 1 + Zend/zend_globals.h | 4 ++++ Zend/zend_objects_API.c | 9 ++++----- main/main.c | 2 ++ 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Zend/zend.c b/Zend/zend.c index bcbca562b198b..c5236ba7ea6dd 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -648,6 +648,7 @@ static void executor_globals_ctor(zend_executor_globals *executor_globals) /* {{ #ifdef ZEND_WIN32 zend_get_windows_version_info(&executor_globals->windows_version_info); #endif + executor_globals->flags = EG_FLAGS_INITIAL; } /* }}} */ diff --git a/Zend/zend_globals.h b/Zend/zend_globals.h index 083875fc2ce37..3440edfc69bbc 100644 --- a/Zend/zend_globals.h +++ b/Zend/zend_globals.h @@ -214,6 +214,7 @@ struct _zend_executor_globals { zend_bool active; zend_bool valid_symbol_table; + zend_uchar flags; zend_long assertions; @@ -235,6 +236,9 @@ struct _zend_executor_globals { void *reserved[ZEND_MAX_RESERVED_RESOURCES]; }; +#define EG_FLAGS_INITIAL 0x00 +#define EG_FLAGS_IN_SHUTDOWN 0x01 + struct _zend_ini_scanner_globals { zend_file_handle *yy_in; zend_file_handle *yy_out; diff --git a/Zend/zend_objects_API.c b/Zend/zend_objects_API.c index b64d413fc233f..3aedba1bda3f6 100644 --- a/Zend/zend_objects_API.c +++ b/Zend/zend_objects_API.c @@ -26,15 +26,12 @@ #include "zend_API.h" #include "zend_objects_API.h" -ZEND_TLS zend_objects_store_no_reuse; /* Would make more sense to make it a member of zend_objects_store, defining as a global to not break alignments */ - ZEND_API void zend_objects_store_init(zend_objects_store *objects, uint32_t init_size) { objects->object_buckets = (zend_object **) emalloc(init_size * sizeof(zend_object*)); objects->top = 1; /* Skip 0 so that handles are true */ objects->size = init_size; objects->free_list_head = -1; - zend_objects_store_no_reuse = 0; memset(&objects->object_buckets[0], 0, sizeof(zend_object*)); } @@ -46,7 +43,6 @@ ZEND_API void zend_objects_store_destroy(zend_objects_store *objects) ZEND_API void zend_objects_store_call_destructors(zend_objects_store *objects) { - zend_objects_store_no_reuse = 1; /* new objects spawned by dtors will never reuse unused slots, so their own dtors will be called further down the loop */ if (objects->top > 1) { uint32_t i; for (i = 1; i < objects->top; i++) { @@ -115,7 +111,10 @@ ZEND_API void zend_objects_store_put(zend_object *object) { int handle; - if (!zend_objects_store_no_reuse && EG(objects_store).free_list_head != -1) { + /* When in shutdown sequesnce - do not reuse previously freed handles, to make sure + * the dtors for newly created objects are called in zend_objects_store_call_destructors() loop + */ + if (!(EG(flags) & EG_FLAGS_IN_SHUTDOWN) && EG(objects_store).free_list_head != -1) { handle = EG(objects_store).free_list_head; EG(objects_store).free_list_head = GET_OBJ_BUCKET_NUMBER(EG(objects_store).object_buckets[handle]); } else { diff --git a/main/main.c b/main/main.c index a8674a5d11784..0d847638237fb 100644 --- a/main/main.c +++ b/main/main.c @@ -1798,6 +1798,8 @@ void php_request_shutdown_for_hook(void *dummy) void php_request_shutdown(void *dummy) { zend_bool report_memleaks; + + EG(flags) |= EG_FLAGS_IN_SHUTDOWN; report_memleaks = PG(report_memleaks);