Fix #81243: Too much memory is allocated for preg_replace()

Trimming a potentially over-allocated string appears to be reasonable, so we drop the condition altogether. We also re-allocate twice the size needed in the first place, and not roughly tripple the size. Closes GH-7231.
php · Jul 12, 2021 · a6b4308 · a6b4308
1 parent bb43aa2
commit a6b4308
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 18 deletions.
diff --git a/NEWS b/NEWS
@@ -25,6 +25,7 @@ PHP                                                                        NEWS
 
 - PCRE:
   . Fixed bug #81101 (PCRE2 10.37 shows unexpected result). (Anatol)
+  . Fixed bug #81243 (Too much memory is allocated for preg_replace()). (cmb)
 
 - Standard:
   . Fixed bug #81223 (flock() only locks first byte of file). (cmb)

diff --git a/ext/pcre/php_pcre.c b/ext/pcre/php_pcre.c
@@ -1719,7 +1719,7 @@ PHPAPI zend_string *php_pcre_replace_impl(pcre_cache_entry *pce, zend_string *su
 			}
 
 			if (new_len >= alloc_len) {
-				alloc_len = zend_safe_address_guarded(2, new_len, alloc_len);
+				alloc_len = zend_safe_address_guarded(2, new_len, 0);
 				if (result == NULL) {
 					result = zend_string_alloc(alloc_len, 0);
 				} else {
@@ -1805,14 +1805,12 @@ PHPAPI zend_string *php_pcre_replace_impl(pcre_cache_entry *pce, zend_string *su
 				result = zend_string_copy(subject_str);
 				break;
 			}
-			new_len = result_len + subject_len - last_end_offset;
-			if (new_len >= alloc_len) {
-				alloc_len = new_len; /* now we know exactly how long it is */
-				if (NULL != result) {
-					result = zend_string_realloc(result, alloc_len, 0);
-				} else {
-					result = zend_string_alloc(alloc_len, 0);
-				}
+			/* now we know exactly how long it is */
+			alloc_len = result_len + subject_len - last_end_offset;
+			if (NULL != result) {
+				result = zend_string_realloc(result, alloc_len, 0);
+			} else {
+				result = zend_string_alloc(alloc_len, 0);
 			}
 			/* stick that last bit of string on our output */
 			memcpy(ZSTR_VAL(result) + result_len, piece, subject_len - last_end_offset);
@@ -1959,7 +1957,7 @@ static zend_string *php_pcre_replace_func_impl(pcre_cache_entry *pce, zend_strin
 			ZEND_ASSERT(eval_result);
 			new_len = zend_safe_address_guarded(1, ZSTR_LEN(eval_result), new_len);
 			if (new_len >= alloc_len) {
-				alloc_len = zend_safe_address_guarded(2, new_len, alloc_len);
+				alloc_len = zend_safe_address_guarded(2, new_len, 0);
 				if (result == NULL) {
 					result = zend_string_alloc(alloc_len, 0);
 				} else {
@@ -2016,14 +2014,12 @@ static zend_string *php_pcre_replace_func_impl(pcre_cache_entry *pce, zend_strin
 				result = zend_string_copy(subject_str);
 				break;
 			}
-			new_len = result_len + subject_len - last_end_offset;
-			if (new_len >= alloc_len) {
-				alloc_len = new_len; /* now we know exactly how long it is */
-				if (NULL != result) {
-					result = zend_string_realloc(result, alloc_len, 0);
-				} else {
-					result = zend_string_alloc(alloc_len, 0);
-				}
+			/* now we know exactly how long it is */
+			alloc_len = result_len + subject_len - last_end_offset;
+			if (NULL != result) {
+				result = zend_string_realloc(result, alloc_len, 0);
+			} else {
+				result = zend_string_alloc(alloc_len, 0);
 			}
 			/* stick that last bit of string on our output */
 			memcpy(ZSTR_VAL(result) + result_len, piece, subject_len - last_end_offset);

diff --git a/ext/pcre/tests/bug81243.phpt b/ext/pcre/tests/bug81243.phpt
@@ -0,0 +1,21 @@
+--TEST--
+Bug #81243 (Too much memory is allocated for preg_replace())
+--FILE--
+<?php
+$test_string = str_repeat('Eins zwei drei', 2000);
+
+$replaced = preg_replace('/\s/', '-', $test_string);
+$mem0 = memory_get_usage();
+$replaced = str_repeat($replaced, 1);
+$mem1 = memory_get_usage();
+var_dump($mem0 == $mem1);
+
+$replaced = preg_replace_callback('/\s/', function ($_) {return '-';}, $test_string);
+$mem0 = memory_get_usage();
+$replaced = str_repeat($replaced, 1);
+$mem1 = memory_get_usage();
+var_dump($mem0 == $mem1);
+?>
+--EXPECT--
+bool(true)
+bool(true)