Fix phpGH-17122: memory leak in regex

Because the subpattern names are persistent, and the fact that the
symbol table destruction is skipped when using fast_shutdown,
this means the refcounts will not be updated for the destruction of
the arrays that hold the subpattern name keys.
To solve this, detect this situation and duplicate the strings.

Closes phpGH-17132.

Author: ndossche

ext/pcre/php_pcre.c

@@ -49,10 +49,14 @@ char *php_pcre_version;
 
 struct _pcre_cache_entry {
 	pcre2_code *re;
-	/* Pointer is not NULL when there are named captures.
-	 * Length is equal to capture_count + 1 to account for capture group 0. */
+	/* Pointer is not NULL (during request) when there are named captures.
+	 * Length is equal to capture_count + 1 to account for capture group 0.
+	 * This table cache is only valid during request.
+	 * Trying to store this over multiple requests causes issues when the keys are exposed in user arrays
+	 * (see GH-17122 and GH-17132). */
 	zend_string **subpats_table;
 	uint32_t preg_options;
+	uint32_t name_count;
 	uint32_t capture_count;
 	uint32_t compile_options;
 	uint32_t refcount;
@@ -478,6 +482,14 @@ static PHP_RINIT_FUNCTION(pcre)
 
 static PHP_RSHUTDOWN_FUNCTION(pcre)
 {
+	pcre_cache_entry *pce;
+	ZEND_HASH_MAP_FOREACH_PTR(&PCRE_G(pcre_cache), pce) {
+		if (pce->subpats_table) {
+			free_subpats_table(pce->subpats_table, pce->capture_count + 1);
+			pce->subpats_table = NULL;
+		}
+	} ZEND_HASH_FOREACH_END();
+
 	pcre2_general_context_free(PCRE_G(gctx_zmm));
 	PCRE_G(gctx_zmm) = NULL;
 
@@ -509,10 +521,10 @@ static void free_subpats_table(zend_string **subpat_names, uint32_t num_subpats)
 	uint32_t i;
 	for (i = 0; i < num_subpats; i++) {
 		if (subpat_names[i]) {
-			zend_string_release_ex(subpat_names[i], true);
+			zend_string_release_ex(subpat_names[i], false);
 		}
 	}
-	pefree(subpat_names, true);
+	efree(subpat_names);
 }
 
 /* {{{ static make_subpats_table */
@@ -531,24 +543,25 @@ static zend_string **make_subpats_table(uint32_t name_cnt, pcre_cache_entry *pce
 		return NULL;
 	}
 
-	subpat_names = pecalloc(num_subpats, sizeof(zend_string *), true);
+	subpat_names = ecalloc(num_subpats, sizeof(zend_string *));
 	while (ni++ < name_cnt) {
 		unsigned short name_idx = 0x100 * (unsigned char)name_table[0] + (unsigned char)name_table[1];
 		const char *name = name_table + 2;
-		/* Note: this makes a persistent string when the cache is not request-based because the string
-		 * has to outlive the request. In that case, they will only be used within this thread
-		 * and never be shared.
-		 * Although we will be storing them in user-exposed arrays, they cannot cause problems
-		 * because they only live in this thread and the last reference is deleted on shutdown
-		 * instead of by user code. */
-		subpat_names[name_idx] = zend_string_init(name, strlen(name), true);
-		GC_MAKE_PERSISTENT_LOCAL(subpat_names[name_idx]);
+		subpat_names[name_idx] = zend_string_init(name, strlen(name), false);
 		name_table += name_size;
 	}
 	return subpat_names;
 }
 /* }}} */
 
+static zend_string **ensure_subpats_table(uint32_t name_cnt, pcre_cache_entry *pce)
+{
+	if (!pce->subpats_table) {
+		pce->subpats_table = make_subpats_table(name_cnt, pce);
+	}
+	return pce->subpats_table;
+}
+
 /* {{{ static calculate_unit_length */
 /* Calculates the byte length of the next character. Assumes valid UTF-8 for PCRE2_UTF. */
 static zend_always_inline size_t calculate_unit_length(pcre_cache_entry *pce, const char *start)
@@ -820,6 +833,7 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache_ex(zend_string *regex, bo
 	new_entry.preg_options = poptions;
 	new_entry.compile_options = coptions;
 	new_entry.refcount = 0;
+	new_entry.subpats_table = NULL;
 
 	rc = pcre2_pattern_info(re, PCRE2_INFO_CAPTURECOUNT, &new_entry.capture_count);
 	if (rc < 0) {
@@ -831,8 +845,7 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache_ex(zend_string *regex, bo
 		return NULL;
 	}
 
-	uint32_t name_count;
-	rc = pcre2_pattern_info(re, PCRE2_INFO_NAMECOUNT, &name_count);
+	rc = pcre2_pattern_info(re, PCRE2_INFO_NAMECOUNT, &new_entry.name_count);
 	if (rc < 0) {
 		if (key != regex) {
 			zend_string_release_ex(key, 0);
@@ -842,21 +855,6 @@ PHPAPI pcre_cache_entry* pcre_get_compiled_regex_cache_ex(zend_string *regex, bo
 		return NULL;
 	}
 
-	/* Compute and cache the subpattern table to avoid computing it again over and over. */
-	if (name_count > 0) {
-		new_entry.subpats_table = make_subpats_table(name_count, &new_entry);
-		if (!new_entry.subpats_table) {
-			if (key != regex) {
-				zend_string_release_ex(key, false);
-			}
-			/* Warning already emitted by make_subpats_table() */
-			pcre_handle_exec_error(PCRE2_ERROR_INTERNAL);
-			return NULL;
-		}
-	} else {
-		new_entry.subpats_table = NULL;
-	}
-
 	/*
 	 * Interned strings are not duplicated when stored in HashTable,
 	 * but all the interned strings created during HTTP request are removed
@@ -971,6 +969,8 @@ static zend_always_inline void populate_match_value(
 
 static inline void add_named(
 		HashTable *const subpats, zend_string *name, zval *val, bool unmatched) {
+	ZEND_ASSERT(!(GC_FLAGS(name) & IS_STR_PERSISTENT));
+
 	/* If the DUPNAMES option is used, multiple subpatterns might have the same name.
 	 * In this case we want to preserve the one that actually has a value. */
 	if (!unmatched) {
@@ -1232,8 +1232,11 @@ PHPAPI void php_pcre_match_impl(pcre_cache_entry *pce, zend_string *subject_str,
 	 * allocate the table only if there are any named subpatterns.
 	 */
 	subpat_names = NULL;
-	if (subpats) {
-		subpat_names = pce->subpats_table;
+	if (subpats && pce->name_count > 0) {
+		subpat_names = ensure_subpats_table(pce->name_count, pce);
+		if (UNEXPECTED(!subpat_names)) {
+			RETURN_FALSE;
+		}
 	}
 
 	matched = 0;
@@ -1869,7 +1872,14 @@ static zend_string *php_pcre_replace_func_impl(pcre_cache_entry *pce, zend_strin
 
 	/* Calculate the size of the offsets array, and allocate memory for it. */
 	num_subpats = pce->capture_count + 1;
-	subpat_names = pce->subpats_table;
+	if (pce->name_count > 0) {
+		subpat_names = ensure_subpats_table(pce->name_count, pce);
+		if (UNEXPECTED(!subpat_names)) {
+			return NULL;
+		}
+	} else {
+		subpat_names = NULL;
+	}
 
 	alloc_len = 0;
 	result = NULL;

ext/pcre/tests/gh17122.phpt

@@ -0,0 +1,30 @@
+--TEST--
+GH-17122 (memory leak in regex)
+--FILE--
+<?php
+preg_match('|(?P<name>)(\d+)|', 0xffffffff, $m1);
+var_dump($m1);
+\preg_match('|(?P<name2>)(\d+)|', 0, $m2);
+var_dump($m2);
+?>
+--EXPECT--
+array(4) {
+  [0]=>
+  string(10) "4294967295"
+  ["name"]=>
+  string(0) ""
+  [1]=>
+  string(0) ""
+  [2]=>
+  string(10) "4294967295"
+}
+array(4) {
+  [0]=>
+  string(1) "0"
+  ["name2"]=>
+  string(0) ""
+  [1]=>
+  string(0) ""
+  [2]=>
+  string(1) "0"
+}