From 7f20fad5cc62522da45b0bbd06dad64de0860aed Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 10 Nov 2024 23:22:53 +0100 Subject: [PATCH] Fix GH-16727: Opcache bad signal 139 crash in ZTS bookworm (frankenphp) Reproducer: https://github.com/php/php-src/issues/16727#issuecomment-2466256317 The root cause is a data race between two different threads: 1) We allocate a lower cased name for an anonymous class here: https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8109 2) This gets looked up as an interned string here: https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8112 Assuming that there are uppercase symbols in the string and therefore `lcname != name` and that `lcname` is not yet in the interned string table, the pointer value of `lcname` won't change. 3) Here we add the string into the interned string table: https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8223 However, in the meantime another thread could've added the string into the interned string table. This means that the following code will run, indirectly called via the `LITERAL_STR` macro, freeing `lcname`: https://github.com/php/php-src/blob/62e53e6f4965f37d379a3fd21f65a4210c5c86b5/ext/opcache/ZendAccelerator.c#L572-L575 4) In the reproducer we then access the freed `lcname` string here: https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8229 This is solved in my patch by retrieving the interned string pointer and putting it in `lcname`. --- Zend/zend_compile.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 5e79152f2578e..38d378a4175bb 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -8043,7 +8043,13 @@ static void zend_compile_class_decl(znode *result, zend_ast *ast, bool toplevel) } opline->op1_type = IS_CONST; - LITERAL_STR(opline->op1, lcname); + /* It's possible that `lcname` is not an interned string because it was not yet in the interned string table. + * However, by this point another thread may have caused `lcname` to be added in the interned string table. + * This will cause `lcname` to get freed once it is found in the interned string table. If we were to use + * LITERAL_STR() here we would not change the `lcname` pointer to the new value, and it would point to the + * now-freed string. This will cause issues when we use `lcname` in the code below. We solve this by using + * zend_add_literal_string() which gives us the new value. */ + opline->op1.constant = zend_add_literal_string(&lcname); if (decl->flags & ZEND_ACC_ANON_CLASS) { opline->opcode = ZEND_DECLARE_ANON_CLASS;