Fix bug #81142 by adding zend_string_init_existing_interned()

nikic · nikic · commit 4a4ae45a0bcb · 2021-08-12T11:57:50.000+02:00
Add a new interned string handler that fetches an interned string if it exists, but does not create one if it does not (and instead returns a non-interned string). This fixes bug #81142, by preventing the creating of new interned strings for unserialized array keys. Closes GH-7360.
diff --git a/NEWS b/NEWS
@@ -34,6 +34,10 @@ PHP                                                                        NEWS
 - SNMP:
   . Implement SHA256 and SHA512 for security protocol. (remi)
 
+- Standard:
+  . Fixed bug #81142 (PHP 7.3+ memory leak when unserialize() is used on an
+    associative array). (Nikita)
+
 05 Aug 2021, PHP 8.1.0beta2
 
 - Core:
diff --git a/Zend/zend_string.c b/Zend/zend_string.c
@@ -25,11 +25,14 @@
 
 ZEND_API zend_new_interned_string_func_t zend_new_interned_string;
 ZEND_API zend_string_init_interned_func_t zend_string_init_interned;
+ZEND_API zend_string_init_existing_interned_func_t zend_string_init_existing_interned;
 
 static zend_string* ZEND_FASTCALL zend_new_interned_string_permanent(zend_string *str);
 static zend_string* ZEND_FASTCALL zend_new_interned_string_request(zend_string *str);
 static zend_string* ZEND_FASTCALL zend_string_init_interned_permanent(const char *str, size_t size, bool permanent);
+static zend_string* ZEND_FASTCALL zend_string_init_existing_interned_permanent(const char *str, size_t size, bool permanent);
 static zend_string* ZEND_FASTCALL zend_string_init_interned_request(const char *str, size_t size, bool permanent);
+static zend_string* ZEND_FASTCALL zend_string_init_existing_interned_request(const char *str, size_t size, bool permanent);
 
 /* Any strings interned in the startup phase. Common to all the threads,
    won't be free'd until process exit. If we want an ability to
@@ -39,6 +42,7 @@ static HashTable interned_strings_permanent;
 
 static zend_new_interned_string_func_t interned_string_request_handler = zend_new_interned_string_request;
 static zend_string_init_interned_func_t interned_string_init_request_handler = zend_string_init_interned_request;
+static zend_string_init_existing_interned_func_t interned_string_init_existing_request_handler = zend_string_init_existing_interned_request;
 
 ZEND_API zend_string  *zend_empty_string = NULL;
 ZEND_API zend_string  *zend_one_char_string[256];
@@ -83,6 +87,7 @@ ZEND_API void zend_interned_strings_init(void)
 
 	interned_string_request_handler = zend_new_interned_string_request;
 	interned_string_init_request_handler = zend_string_init_interned_request;
+	interned_string_init_existing_request_handler = zend_string_init_existing_interned_request;
 
 	zend_empty_string = NULL;
 	zend_known_strings = NULL;
@@ -91,6 +96,7 @@ ZEND_API void zend_interned_strings_init(void)
 
 	zend_new_interned_string = zend_new_interned_string_permanent;
 	zend_string_init_interned = zend_string_init_interned_permanent;
+	zend_string_init_existing_interned = zend_string_init_existing_interned_permanent;
 
 	/* interned empty string */
 	str = zend_string_alloc(sizeof("")-1, 1);
@@ -267,6 +273,20 @@ static zend_string* ZEND_FASTCALL zend_string_init_interned_permanent(const char
 	return zend_add_interned_string(ret, &interned_strings_permanent, IS_STR_PERMANENT);
 }
 
+static zend_string* ZEND_FASTCALL zend_string_init_existing_interned_permanent(const char *str, size_t size, bool permanent)
+{
+	zend_ulong h = zend_inline_hash_func(str, size);
+	zend_string *ret = zend_interned_string_ht_lookup_ex(h, str, size, &interned_strings_permanent);
+	if (ret) {
+		return ret;
+	}
+
+	ZEND_ASSERT(permanent);
+	ret = zend_string_init(str, size, permanent);
+	ZSTR_H(ret) = h;
+	return ret;
+}
+
 static zend_string* ZEND_FASTCALL zend_string_init_interned_request(const char *str, size_t size, bool permanent)
 {
 	zend_string *ret;
@@ -297,6 +317,25 @@ static zend_string* ZEND_FASTCALL zend_string_init_interned_request(const char *
 	return zend_add_interned_string(ret, &CG(interned_strings), 0);
 }
 
+static zend_string* ZEND_FASTCALL zend_string_init_existing_interned_request(const char *str, size_t size, bool permanent)
+{
+	zend_ulong h = zend_inline_hash_func(str, size);
+	zend_string *ret = zend_interned_string_ht_lookup_ex(h, str, size, &interned_strings_permanent);
+	if (ret) {
+		return ret;
+	}
+
+	ret = zend_interned_string_ht_lookup_ex(h, str, size, &CG(interned_strings));
+	if (ret) {
+		return ret;
+	}
+
+	ZEND_ASSERT(!permanent);
+	ret = zend_string_init(str, size, permanent);
+	ZSTR_H(ret) = h;
+	return ret;
+}
+
 ZEND_API void zend_interned_strings_activate(void)
 {
 	zend_init_interned_strings_ht(&CG(interned_strings), 0);
@@ -307,20 +346,23 @@ ZEND_API void zend_interned_strings_deactivate(void)
 	zend_hash_destroy(&CG(interned_strings));
 }
 
-ZEND_API void zend_interned_strings_set_request_storage_handlers(zend_new_interned_string_func_t handler, zend_string_init_interned_func_t init_handler)
+ZEND_API void zend_interned_strings_set_request_storage_handlers(zend_new_interned_string_func_t handler, zend_string_init_interned_func_t init_handler, zend_string_init_existing_interned_func_t init_existing_handler)
 {
 	interned_string_request_handler = handler;
 	interned_string_init_request_handler = init_handler;
+	interned_string_init_existing_request_handler = init_existing_handler;
 }
 
 ZEND_API void zend_interned_strings_switch_storage(bool request)
 {
 	if (request) {
 		zend_new_interned_string = interned_string_request_handler;
 		zend_string_init_interned = interned_string_init_request_handler;
+		zend_string_init_existing_interned = interned_string_init_existing_request_handler;
 	} else {
 		zend_new_interned_string = zend_new_interned_string_permanent;
 		zend_string_init_interned = zend_string_init_interned_permanent;
+		zend_string_init_existing_interned = zend_string_init_existing_interned_permanent;
 	}
 }
 
diff --git a/Zend/zend_string.h b/Zend/zend_string.h
@@ -26,9 +26,12 @@ BEGIN_EXTERN_C()
 typedef void (*zend_string_copy_storage_func_t)(void);
 typedef zend_string *(ZEND_FASTCALL *zend_new_interned_string_func_t)(zend_string *str);
 typedef zend_string *(ZEND_FASTCALL *zend_string_init_interned_func_t)(const char *str, size_t size, bool permanent);
+typedef zend_string *(ZEND_FASTCALL *zend_string_init_existing_interned_func_t)(const char *str, size_t size, bool permanent);
 
 ZEND_API extern zend_new_interned_string_func_t zend_new_interned_string;
 ZEND_API extern zend_string_init_interned_func_t zend_string_init_interned;
+/* Init an interned string if it already exists, but do not create a new one if it does not. */
+ZEND_API extern zend_string_init_existing_interned_func_t zend_string_init_existing_interned;
 
 ZEND_API zend_ulong ZEND_FASTCALL zend_string_hash_func(zend_string *str);
 ZEND_API zend_ulong ZEND_FASTCALL zend_hash_func(const char *str, size_t len);
@@ -46,7 +49,10 @@ ZEND_API void zend_interned_strings_init(void);
 ZEND_API void zend_interned_strings_dtor(void);
 ZEND_API void zend_interned_strings_activate(void);
 ZEND_API void zend_interned_strings_deactivate(void);
-ZEND_API void zend_interned_strings_set_request_storage_handlers(zend_new_interned_string_func_t handler, zend_string_init_interned_func_t init_handler);
+ZEND_API void zend_interned_strings_set_request_storage_handlers(
+	zend_new_interned_string_func_t handler,
+	zend_string_init_interned_func_t init_handler,
+	zend_string_init_existing_interned_func_t init_existing_handler);
 ZEND_API void zend_interned_strings_switch_storage(bool request);
 
 ZEND_API extern zend_string  *zend_empty_string;
diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c
@@ -2878,7 +2878,12 @@ static int zend_accel_init_shm(void)
 		*STRTAB_HASH_TO_SLOT(&ZCSG(interned_strings), 0) = STRTAB_INVALID_POS;
 	}
 
-	zend_interned_strings_set_request_storage_handlers(accel_new_interned_string_for_php, accel_init_interned_string_for_php);
+	/* We can reuse init_interned_string_for_php for the "init_existing_interned" case,
+	 * because the function does not create new interned strings at runtime. */
+	zend_interned_strings_set_request_storage_handlers(
+		accel_new_interned_string_for_php,
+		accel_init_interned_string_for_php,
+		accel_init_interned_string_for_php);
 
 	zend_reset_cache_vars();
 
@@ -3198,7 +3203,10 @@ static zend_result accel_post_startup(void)
 #endif
 				zend_shared_alloc_lock();
 				accel_shared_globals = (zend_accel_shared_globals *) ZSMMG(app_shared_globals);
-				zend_interned_strings_set_request_storage_handlers(accel_new_interned_string_for_php, accel_init_interned_string_for_php);
+				zend_interned_strings_set_request_storage_handlers(
+					accel_new_interned_string_for_php,
+					accel_init_interned_string_for_php,
+					accel_init_interned_string_for_php);
 				zend_shared_alloc_unlock();
 				break;
 			case FAILED_REATTACHED:
diff --git a/ext/standard/tests/serialize/bug81142.phpt b/ext/standard/tests/serialize/bug81142.phpt
@@ -0,0 +1,12 @@
+--TEST--
+Bug #81142 (memory leak when unserialize()ing associative array)
+--FILE--
+<?php
+$mem0 = memory_get_usage();
+$ctr = 0;
+unserialize(serialize(["foo_$ctr" => 1]));
+$mem1 = memory_get_usage();
+var_dump($mem1 - $mem0);
+?>
+--EXPECT--
+int(0)
diff --git a/ext/standard/var_unserializer.re b/ext/standard/var_unserializer.re
@@ -1024,7 +1024,7 @@ use_double:
 
 	if (!var_hash) {
 		/* Array or object key unserialization */
-		ZVAL_STR(rval, zend_string_init_interned(str, len, 0));
+		ZVAL_STR(rval, zend_string_init_existing_interned(str, len, 0));
 	} else {
 		ZVAL_STRINGL_FAST(rval, str, len);
 	}

Original file line number	Diff line number	Diff line change
`@@ -1024,7 +1024,7 @@ use_double:`
`1024`	`1024`
`1025`	`1025`	`if (!var_hash) {`
`1026`	`1026`	`/* Array or object key unserialization */`
`1027`		`- ZVAL_STR(rval, zend_string_init_interned(str, len, 0));`
	`1027`	`+ ZVAL_STR(rval, zend_string_init_existing_interned(str, len, 0));`
`1028`	`1028`	`} else {`
`1029`	`1029`	`ZVAL_STRINGL_FAST(rval, str, len);`
`1030`	`1030`	`}`