From 0f2cdbf214efd98b4bdaf5ca41728faf00e7c037 Mon Sep 17 00:00:00 2001
From: Nikita Popov <nikita.ppv@gmail.com>
Date: Wed, 11 Dec 2019 17:11:30 +0100
Subject: [PATCH] Introduce extra counter to avoid RTD key collisions
Also generate a fatal error if a collision occurs in zend_compile.
This is not perfect, because collisions might still be introduced
via opcache, if one file is included multiple times during a request,
invalidate in the meantime and recompiled by different processes.
This still needs to be addressed, but this patch fixes the much
more common case of collisions occuring when opcache is not used.
Fixes bug #78903.
---
NEWS | 2 +
Zend/zend.c | 2 +
Zend/zend_compile.c | 46 ++++++++++-------------
Zend/zend_globals.h | 2 +
ext/opcache/zend_accelerator_util_funcs.c | 20 +++++++---
5 files changed, 39 insertions(+), 33 deletions(-)
diff --git a/NEWS b/NEWS
index 7372fb6a87..8bcd28fe5a 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,8 @@ PHP NEWS
- OPcache:
. Fixed bug #78950 (Preloading trait method with static variables). (Nikita)
+ . Fixed bug #78903 (Conflict in RTD key for closures results in crash).
+ (Nikita)
19 Dec 2019, PHP 7.4.1
diff --git a/Zend/zend.c b/Zend/zend.c
index af4ac2981c..97ac3327ca 100644
--- a/Zend/zend.c
+++ b/Zend/zend.c
@@ -524,6 +524,8 @@ static void zend_set_default_compile_time_values(void) /* {{{ */
/* default compile-time values */
CG(short_tags) = short_tags_default;
CG(compiler_options) = compiler_options_default;
+
+ CG(rtd_key_counter) = 0;
}
/* }}} */
diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 8feb13955d..d8e051c1a1 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -127,16 +127,11 @@ static void zend_destroy_property_info_internal(zval *zv) /* {{{ */
}
/* }}} */
-static zend_string *zend_build_runtime_definition_key(zend_string *name, unsigned char *lex_pos) /* {{{ */
+static zend_string *zend_build_runtime_definition_key(zend_string *name, uint32_t start_lineno) /* {{{ */
{
- zend_string *result;
- char char_pos_buf[32];
- size_t char_pos_len = sprintf(char_pos_buf, "%p", lex_pos);
zend_string *filename = CG(active_op_array)->filename;
-
- /* NULL, name length, filename length, last accepting char position length */
- result = zend_string_alloc(1 + ZSTR_LEN(name) + ZSTR_LEN(filename) + char_pos_len, 0);
- sprintf(ZSTR_VAL(result), "%c%s%s%s", '\0', ZSTR_VAL(name), ZSTR_VAL(filename), char_pos_buf);
+ zend_string *result = zend_strpprintf(0, "%c%s%s:%" PRIu32 "$%" PRIx32,
+ '\0', ZSTR_VAL(name), ZSTR_VAL(filename), start_lineno, CG(rtd_key_counter)++);
return zend_new_interned_string(result);
}
/* }}} */
@@ -5924,8 +5919,11 @@ static void zend_begin_func_decl(znode *result, zend_op_array *op_array, zend_as
return;
}
- key = zend_build_runtime_definition_key(lcname, decl->lex_pos);
- zend_hash_update_ptr(CG(function_table), key, op_array);
+ key = zend_build_runtime_definition_key(lcname, decl->start_lineno);
+ if (!zend_hash_add_ptr(CG(function_table), key, op_array)) {
+ zend_error_noreturn(E_ERROR,
+ "Runtime definition key collision for function %s. This is a bug", ZSTR_VAL(name));
+ }
if (op_array->fn_flags & ZEND_ACC_CLOSURE) {
opline = zend_emit_op_tmp(result, ZEND_DECLARE_LAMBDA_FUNCTION, NULL, NULL);
@@ -6347,16 +6345,11 @@ void zend_compile_implements(zend_ast *ast) /* {{{ */
}
/* }}} */
-static zend_string *zend_generate_anon_class_name(unsigned char *lex_pos) /* {{{ */
+static zend_string *zend_generate_anon_class_name(uint32_t start_lineno) /* {{{ */
{
- zend_string *result;
- char char_pos_buf[32];
- size_t char_pos_len = sprintf(char_pos_buf, "%p", lex_pos);
zend_string *filename = CG(active_op_array)->filename;
-
- /* NULL, name length, filename length, last accepting char position length */
- result = zend_string_alloc(sizeof("class@anonymous") + ZSTR_LEN(filename) + char_pos_len, 0);
- sprintf(ZSTR_VAL(result), "class@anonymous%c%s%s", '\0', ZSTR_VAL(filename), char_pos_buf);
+ zend_string *result = zend_strpprintf(0, "class@anonymous%c%s:%" PRIu32 "$%" PRIx32,
+ '\0', ZSTR_VAL(filename), start_lineno, CG(rtd_key_counter)++);
return zend_new_interned_string(result);
}
/* }}} */
@@ -6396,7 +6389,7 @@ zend_op *zend_compile_class_decl(zend_ast *ast, zend_bool toplevel) /* {{{ */
zend_register_seen_symbol(lcname, ZEND_SYMBOL_CLASS);
} else {
- name = zend_generate_anon_class_name(decl->lex_pos);
+ name = zend_generate_anon_class_name(decl->start_lineno);
lcname = zend_string_tolower(name);
}
lcname = zend_new_interned_string(lcname);
@@ -6553,20 +6546,19 @@ zend_op *zend_compile_class_decl(zend_ast *ast, zend_bool toplevel) /* {{{ */
opline->extended_value = zend_alloc_cache_slot();
opline->result_type = IS_VAR;
opline->result.var = get_temporary_variable();
-
if (!zend_hash_add_ptr(CG(class_table), lcname, ce)) {
- /* this anonymous class has been included */
- zval zv;
- ZVAL_PTR(&zv, ce);
- destroy_zend_class(&zv);
- return opline;
+ zend_error_noreturn(E_ERROR,
+ "Runtime definition key collision for %s. This is a bug", ZSTR_VAL(name));
}
} else {
- zend_string *key = zend_build_runtime_definition_key(lcname, decl->lex_pos);
+ zend_string *key = zend_build_runtime_definition_key(lcname, decl->start_lineno);
/* RTD key is placed after lcname literal in op1 */
zend_add_literal_string(&key);
- zend_hash_update_ptr(CG(class_table), key, ce);
+ if (!zend_hash_add_ptr(CG(class_table), key, ce)) {
+ zend_error_noreturn(E_ERROR,
+ "Runtime definition key collision for class %s. This is a bug", ZSTR_VAL(name));
+ }
opline->opcode = ZEND_DECLARE_CLASS;
if (extends_ast && toplevel
diff --git a/Zend/zend_globals.h b/Zend/zend_globals.h
index 6332a6f14d..2e9fff40ad 100644
--- a/Zend/zend_globals.h
+++ b/Zend/zend_globals.h
@@ -127,6 +127,8 @@ struct _zend_compiler_globals {
HashTable *delayed_variance_obligations;
HashTable *delayed_autoloads;
+
+ uint32_t rtd_key_counter;
};
diff --git a/ext/opcache/zend_accelerator_util_funcs.c b/ext/opcache/zend_accelerator_util_funcs.c
index dc7a76b326..0eb5b70cc5 100644
--- a/ext/opcache/zend_accelerator_util_funcs.c
+++ b/ext/opcache/zend_accelerator_util_funcs.c
@@ -422,8 +422,16 @@ static void zend_accel_function_hash_copy(HashTable *target, HashTable *source)
t = zend_hash_find_ex(target, p->key, 1);
if (UNEXPECTED(t != NULL)) {
if (EXPECTED(ZSTR_LEN(p->key) > 0) && EXPECTED(ZSTR_VAL(p->key)[0] == 0)) {
- /* Mangled key */
- t = zend_hash_update(target, p->key, &p->val);
+ /* Runtime definition key. There are two circumstances under which the key can
+ * already be defined:
+ * 1. The file has been re-included without being changed in the meantime. In
+ * this case we can keep the old value, because we know that the definition
+ * hasn't changed.
+ * 2. The file has been changed in the meantime, but the RTD key ends up colliding.
+ * This would be a bug.
+ * As we can't distinguish these cases, we assume that it is 1. and keep the old
+ * value. */
+ continue;
} else {
goto failure;
}
@@ -466,8 +474,8 @@ static void zend_accel_function_hash_copy_from_shm(HashTable *target, HashTable
t = zend_hash_find_ex(target, p->key, 1);
if (UNEXPECTED(t != NULL)) {
if (EXPECTED(ZSTR_LEN(p->key) > 0) && EXPECTED(ZSTR_VAL(p->key)[0] == 0)) {
- /* Mangled key */
- zend_hash_update_ptr(target, p->key, Z_PTR(p->val));
+ /* See comment in zend_accel_function_hash_copy(). */
+ continue;
} else {
goto failure;
}
@@ -509,7 +517,7 @@ static void zend_accel_class_hash_copy(HashTable *target, HashTable *source)
t = zend_hash_find_ex(target, p->key, 1);
if (UNEXPECTED(t != NULL)) {
if (EXPECTED(ZSTR_LEN(p->key) > 0) && EXPECTED(ZSTR_VAL(p->key)[0] == 0)) {
- /* Mangled key - ignore and wait for runtime */
+ /* See comment in zend_accel_function_hash_copy(). */
continue;
} else if (UNEXPECTED(!ZCG(accel_directives).ignore_dups)) {
zend_class_entry *ce1 = Z_PTR(p->val);
@@ -546,7 +554,7 @@ static void zend_accel_class_hash_copy_from_shm(HashTable *target, HashTable *so
t = zend_hash_find_ex(target, p->key, 1);
if (UNEXPECTED(t != NULL)) {
if (EXPECTED(ZSTR_LEN(p->key) > 0) && EXPECTED(ZSTR_VAL(p->key)[0] == 0)) {
- /* Mangled key - ignore and wait for runtime */
+ /* See comment in zend_accel_function_hash_copy(). */
continue;
} else if (UNEXPECTED(!ZCG(accel_directives).ignore_dups)) {
zend_class_entry *ce1 = Z_PTR(p->val);
--
2.40.0