From fd6e7f9321060f572310742b9e2947b227f2fbc4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 10 Apr 2024 20:26:16 +0200 Subject: [PATCH 1/6] Fix GH-13931: Applying zero offset to null pointer in Zend/zend_opcode.c In the test cases, the compiler bails out due to a fatal error. The data structures used by the compiler will contain stale values. In particular, for the test case CG(loop_var_stack) will contain data. The next compilation will incorrectly use elements from the previous stack. To solve this, we reset part of the compiler data structures. We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. --- Zend/zend.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Zend/zend.c b/Zend/zend.c index fc092b66b9e2a..4e0a42f3f1e8a 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -1176,9 +1176,19 @@ ZEND_API ZEND_COLD ZEND_NORETURN void _zend_bailout(const char *filename, uint32 exit(-1); } gc_protect(1); + + if (CG(in_compilation)) { + CG(in_compilation) = 0; + /* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack). + * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, + * otherwise this will lead to incorrect compilation during shutdown. + * We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. */ + shutdown_compiler(); + zend_init_compiler_data_structures(); + } + CG(unclean_shutdown) = 1; CG(active_class_entry) = NULL; - CG(in_compilation) = 0; CG(memoize_mode) = 0; EG(current_execute_data) = NULL; LONGJMP(*EG(bailout), FAILURE); From 0615e00a8cc6e126d73ae47a7015accaf5882351 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 10 Apr 2024 21:12:37 +0200 Subject: [PATCH 2/6] Add tests --- Zend/tests/gh13931.phpt | 23 +++++++++++++++++++++++ sapi/phpdbg/tests/gh13931.phpt | 21 +++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 Zend/tests/gh13931.phpt create mode 100644 sapi/phpdbg/tests/gh13931.phpt diff --git a/Zend/tests/gh13931.phpt b/Zend/tests/gh13931.phpt new file mode 100644 index 0000000000000..5f2873b449f94 --- /dev/null +++ b/Zend/tests/gh13931.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-13931 (Applying zero offset to null pointer in Zend/zend_opcode.c) +--FILE-- + +--EXPECTF-- +Fatal error: 'break' not in the 'loop' or 'switch' context in %s on line %d +int(4) diff --git a/sapi/phpdbg/tests/gh13931.phpt b/sapi/phpdbg/tests/gh13931.phpt new file mode 100644 index 0000000000000..25520435feb9b --- /dev/null +++ b/sapi/phpdbg/tests/gh13931.phpt @@ -0,0 +1,21 @@ +--TEST-- +Applying zero offset to null pointer in Zend/zend_opcode.c +--FILE-- + +--PHPDBG-- +ev 1 + 3 +ev 2 ** 3 +q +--EXPECTF-- +Fatal error: 'break' not in the 'loop' or 'switch' context in %s on line %d +prompt> 4 +prompt> 8 +prompt> From 5a6106574f9046a88bcc9e9a1bc9608ad95f8253 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 11 Apr 2024 16:43:44 +0200 Subject: [PATCH 3/6] Move code to php_error_cb --- Zend/zend.c | 12 +----------- main/main.c | 9 +++++++++ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/Zend/zend.c b/Zend/zend.c index 4e0a42f3f1e8a..fc092b66b9e2a 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -1176,19 +1176,9 @@ ZEND_API ZEND_COLD ZEND_NORETURN void _zend_bailout(const char *filename, uint32 exit(-1); } gc_protect(1); - - if (CG(in_compilation)) { - CG(in_compilation) = 0; - /* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack). - * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, - * otherwise this will lead to incorrect compilation during shutdown. - * We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. */ - shutdown_compiler(); - zend_init_compiler_data_structures(); - } - CG(unclean_shutdown) = 1; CG(active_class_entry) = NULL; + CG(in_compilation) = 0; CG(memoize_mode) = 0; EG(current_execute_data) = NULL; LONGJMP(*EG(bailout), FAILURE); diff --git a/main/main.c b/main/main.c index 83f8829890e40..29b7043e34644 100644 --- a/main/main.c +++ b/main/main.c @@ -1405,6 +1405,15 @@ static ZEND_COLD void php_error_cb(int orig_type, zend_string *error_filename, c /* restore memory limit */ zend_set_memory_limit(PG(memory_limit)); zend_objects_store_mark_destructed(&EG(objects_store)); + if (CG(in_compilation)) { + CG(in_compilation) = 0; + /* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack). + * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, + * otherwise this will lead to incorrect compilation during shutdown. + * We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. */ + shutdown_compiler(); + zend_init_compiler_data_structures(); + } zend_bailout(); return; } From 9a8579429c018d6cab9d00a355d021d38d081408 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 12 Apr 2024 13:25:38 +0200 Subject: [PATCH 4/6] Bailout already sets in_compilation=0 --- main/main.c | 1 - 1 file changed, 1 deletion(-) diff --git a/main/main.c b/main/main.c index 29b7043e34644..d1b9b0941aa26 100644 --- a/main/main.c +++ b/main/main.c @@ -1406,7 +1406,6 @@ static ZEND_COLD void php_error_cb(int orig_type, zend_string *error_filename, c zend_set_memory_limit(PG(memory_limit)); zend_objects_store_mark_destructed(&EG(objects_store)); if (CG(in_compilation)) { - CG(in_compilation) = 0; /* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack). * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, * otherwise this will lead to incorrect compilation during shutdown. From e7e216092f603a942ac53c0460418b58b343d332 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 12 Apr 2024 13:32:27 +0200 Subject: [PATCH 5/6] Only recover the compiler on compilation or parse error --- main/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main/main.c b/main/main.c index d1b9b0941aa26..fa87ae98d7295 100644 --- a/main/main.c +++ b/main/main.c @@ -1405,7 +1405,7 @@ static ZEND_COLD void php_error_cb(int orig_type, zend_string *error_filename, c /* restore memory limit */ zend_set_memory_limit(PG(memory_limit)); zend_objects_store_mark_destructed(&EG(objects_store)); - if (CG(in_compilation)) { + if (CG(in_compilation) && (type == E_COMPILE_ERROR || type == E_PARSE)) { /* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack). * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, * otherwise this will lead to incorrect compilation during shutdown. From 438ffdbdf1dcb613220fc612ba5689ea110775f8 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 12 Apr 2024 13:32:30 +0200 Subject: [PATCH 6/6] Fix indentation --- main/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/main/main.c b/main/main.c index fa87ae98d7295..634b00936bc3e 100644 --- a/main/main.c +++ b/main/main.c @@ -1407,9 +1407,9 @@ static ZEND_COLD void php_error_cb(int orig_type, zend_string *error_filename, c zend_objects_store_mark_destructed(&EG(objects_store)); if (CG(in_compilation) && (type == E_COMPILE_ERROR || type == E_PARSE)) { /* We bailout during compilation which may for example leave stale entries in CG(loop_var_stack). - * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, - * otherwise this will lead to incorrect compilation during shutdown. - * We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. */ + * If code is compiled during shutdown, we need to make sure the compiler is reset to a clean state, + * otherwise this will lead to incorrect compilation during shutdown. + * We don't do a full re-initialization via init_compiler() because that will also reset streams and resources. */ shutdown_compiler(); zend_init_compiler_data_structures(); }