mb_decode_numericentity decodes valid entities which are truncated at end of string

Since mb_decode_numericentity does not require all HTML entities
to end with ';', but allows them to be terminated by ANY non-digit
character, it doesn't make sense that valid entities which butt
up against the end of the input string are not converted.

As it turned out, supporting this case also made it possible
to simplify the code nicely.

Author: alexdowad

ext/mbstring/mbstring.c

@@ -3319,6 +3319,11 @@ static bool html_numeric_entity_deconvert(uint32_t number, uint32_t *convmap, in
 	return false;
 }
 
+#define DEC_ENTITY_MINLEN 3  /* For "&#" and 1 decimal digit */
+#define HEX_ENTITY_MINLEN 4  /* For "&#x" and 1 hexadecimal digit */
+#define DEC_ENTITY_MAXLEN 12 /* For "&#" and 10 decimal digits */
+#define HEX_ENTITY_MAXLEN 11 /* For "&#x" and 8 hexadecimal digits */
+
 static zend_string* html_numeric_entity_decode(zend_string *input, const mbfl_encoding *encoding, uint32_t *convmap, int mapsize)
 {
 	uint32_t wchar_buf[128], converted_buf[128];
@@ -3389,28 +3394,20 @@ static zend_string* html_numeric_entity_decode(zend_string *input, const mbfl_en
 				uint32_t w = *++p2;
 				while ((w >= '0' && w <= '9') || (w >= 'A' && w <= 'F') || (w >= 'a' && w <= 'f'))
 					w = *++p2;
-				if (p2 == wchar_buf + out_len) {
-					/* We hit the end of the buffer while still reading digits */
-					if ((p2 - p) <= 11 && in_len) {
-						/* The number of digits was legal (1-8 hex digits)
-						 * We need to process this identity again on the next iteration of the main loop */
-						memmove(wchar_buf, p, (p2 - p) * 4);
-						wchar_buf_offset = p2 - p;
-						goto process_converted_wchars;
-					} else {
-						/* Invalid entity */
-						memcpy(converted, p, (p2 - p) * 4);
-						converted += p2 - p;
-						goto process_converted_wchars_no_offset;
-					}
-				} else if ((p2 - p) == 3 || (p2 - p) > 11) {
-					/* Invalid entity */
+				if ((p2 == wchar_buf + out_len) && in_len && (p2 - p) <= HEX_ENTITY_MAXLEN) {
+					/* We hit the end of the buffer while reading digits, and
+					 * more wchars are still coming in the next buffer
+					 * Reprocess this identity on next iteration */
+					memmove(wchar_buf, p, (p2 - p) * 4);
+					wchar_buf_offset = p2 - p;
+					goto process_converted_wchars;
+				} else if ((p2 - p) < HEX_ENTITY_MINLEN || (p2 - p) > HEX_ENTITY_MAXLEN) {
+					/* Invalid entity (too long or "&#x" only) */
 					memcpy(converted, p, (p2 - p) * 4);
 					converted += p2 - p;
 				} else {
 					/* Valid hexadecimal entity */
-					uint32_t value = 0;
-					uint32_t *p3 = p + 3;
+					uint32_t value = 0, *p3 = p + 3;
 					while (p3 < p2) {
 						w = *p3++;
 						if (w <= '9') {
@@ -3421,48 +3418,33 @@ static zend_string* html_numeric_entity_decode(zend_string *input, const mbfl_en
 							value = (value * 16) + 10 + (w - 'A');
 						}
 					}
-					uint32_t original;
-					if (html_numeric_entity_deconvert(value, convmap, mapsize, &original)) {
-						*converted++ = original;
+					if (html_numeric_entity_deconvert(value, convmap, mapsize, converted)) {
+						converted++;
 						if (*p2 == ';')
 							p2++;
 					} else {
 						memcpy(converted, p, (p2 - p) * 4);
 						converted += p2 - p;
 					}
 				}
-			} else if (p2 == wchar_buf + out_len && in_len) {
-				wchar_buf[0] = '&';
-				wchar_buf[1] = '#';
-				wchar_buf_offset = 2;
-				goto process_converted_wchars;
 			} else {
 				/* Possible decimal entity */
 				uint32_t w = *p2;
 				while (w >= '0' && w <= '9')
 					w = *++p2;
-				if (p2 == wchar_buf + out_len) {
-					if ((p2 - p) <= 12 && in_len) {
-						/* The number of digits was legal (1-10 decimal digits)
-						 * We need to process this identity again on next iteration of main loop */
-						memmove(wchar_buf, p, (p2 - p) * 4);
-						wchar_buf_offset = p2 - p;
-						goto process_converted_wchars;
-					} else {
-						/* Invalid entity */
-						memcpy(converted, p, (p2 - p) * 4);
-						converted += p2 - p;
-						wchar_buf_offset = 0;
-						goto process_converted_wchars_no_offset;
-					}
-				} else if ((p2 - p) == 2 || (p2 - p) > 12) {
-					/* Invalid entity */
+				if ((p2 == wchar_buf + out_len) && in_len && (p2 - p) <= DEC_ENTITY_MAXLEN) {
+					/* The number of digits was legal (no more than 10 decimal digits)
+					 * Reprocess this identity on next iteration of main loop */
+					memmove(wchar_buf, p, (p2 - p) * 4);
+					wchar_buf_offset = p2 - p;
+					goto process_converted_wchars;
+				} else if ((p2 - p) < DEC_ENTITY_MINLEN || (p2 - p) > DEC_ENTITY_MAXLEN) {
+					/* Invalid entity (too long or "&#" only) */
 					memcpy(converted, p, (p2 - p) * 4);
 					converted += p2 - p;
 				} else {
 					/* Valid decimal entity */
-					uint32_t value = 0;
-					uint32_t *p3 = p + 2;
+					uint32_t value = 0, *p3 = p + 2;
 					while (p3 < p2) {
 						/* If unsigned integer overflow would occur in the below
 						 * multiplication by 10, this entity is no good
@@ -3474,9 +3456,8 @@ static zend_string* html_numeric_entity_decode(zend_string *input, const mbfl_en
 						}
 						value = (value * 10) + (*p3++ - '0');
 					}
-					uint32_t original;
-					if (html_numeric_entity_deconvert(value, convmap, mapsize, &original)) {
-						*converted++ = original;
+					if (html_numeric_entity_deconvert(value, convmap, mapsize, converted)) {
+						converted++;
 						if (*p2 == ';')
 							p2++;
 					} else {
@@ -3485,16 +3466,11 @@ static zend_string* html_numeric_entity_decode(zend_string *input, const mbfl_en
 					}
 				}
 			}
-		} else if (p2 == wchar_buf + out_len) {
+		} else if ((p2 == wchar_buf + out_len) && in_len) {
 			/* Corner case: & at end of buffer */
-			if (in_len) {
-				wchar_buf[0] = '&';
-				wchar_buf_offset = 1;
-				goto process_converted_wchars;
-			} else {
-				*converted++ = '&';
-				goto process_converted_wchars_no_offset;
-			}
+			wchar_buf[0] = '&';
+			wchar_buf_offset = 1;
+			goto process_converted_wchars;
 		} else {
 			*converted++ = '&';
 		}
@@ -3515,7 +3491,6 @@ static zend_string* html_numeric_entity_decode(zend_string *input, const mbfl_en
 		if (p < wchar_buf + out_len)
 			goto found_ampersand;
 
-process_converted_wchars_no_offset:
 		/* We do not have any wchars remaining at the end of this buffer which
 		 * we need to reprocess on the next call */
 		wchar_buf_offset = 0;

ext/mbstring/tests/bug40685.phpt

@@ -19,7 +19,7 @@ string(1) "&"
 string(3) "&&&"
 string(2) "&#"
 string(3) "&#x"
-string(4) "&#61"
-string(5) "&#x3d"
+string(1) "="
+string(1) "="
 string(1) "="
 string(1) "="

ext/mbstring/tests/mb_decode_numericentity.phpt

@@ -41,19 +41,19 @@ echo "2: " . mb_decode_numericentity($str2, $convmap, "UTF-8") . "\n";
 echo "3: " . mb_decode_numericentity($str3, $convmap, "UTF-8") . "\n";
 
 // Numeric entities which are truncated at end of string
-// We do NOT decode such entities; they can be terminated by any non-digit character, but not by the end of the string
-echo "4: " . mb_decode_numericentity('&#1000000000', $convmap), "\n";
-echo "5: " . mb_decode_numericentity('&#9000000000', $convmap), "\n";
-echo "6: " . mb_decode_numericentity('&#10000000000', $convmap), "\n";
-echo "7: " . mb_decode_numericentity('&#100000000000', $convmap), "\n";
-echo "8: " . mb_decode_numericentity('&#000000000000', $convmap), "\n";
-echo "9: " . mb_decode_numericentity('&#00000000000', $convmap), "\n";
-echo "10: " . mb_decode_numericentity('&#0000000000', $convmap), "\n";
-echo "11: " . mb_decode_numericentity('&#000000000', $convmap), "\n";
+echo "4: " . mb_decode_numericentity('&#1000000000', $convmap), "\n"; // Entity is too big
+echo "5: " . mb_decode_numericentity('&#9000000000', $convmap), "\n"; // Entity is too big
+echo "6: " . mb_decode_numericentity('&#10000000000', $convmap), "\n"; // Too many digits
+echo "7: " . mb_decode_numericentity('&#100000000000', $convmap), "\n"; // Too many digits
+echo "8: " . mb_decode_numericentity('&#000000000000', $convmap), "\n"; // Too many digits
+echo "9: " . mb_decode_numericentity('&#00000000000', $convmap), "\n"; // Too many digits
+echo "10: " . bin2hex(mb_decode_numericentity('&#0000000000', $convmap)), "\n"; // OK
+echo "11: " . bin2hex(mb_decode_numericentity('&#000000000', $convmap)), "\n"; // OK
 // Try with hex, not just decimal entities
-echo "11b: " . mb_decode_numericentity('&#x0000000', $convmap), "\n";
-echo "11c: " . mb_decode_numericentity('&#x00000000', $convmap), "\n";
-echo "11d: " . mb_decode_numericentity('&#x10000', $convmap), "\n";
+echo "11b: " . bin2hex(mb_decode_numericentity('&#x0000000', $convmap)), "\n"; // OK
+echo "11c: " . bin2hex(mb_decode_numericentity('&#x00000000', $convmap)), "\n"; // OK
+echo "11d: " . bin2hex(mb_decode_numericentity('&#x10000', $convmap)), "\n"; // OK
+echo "11e: " . mb_decode_numericentity('&#x000000000', $convmap), "\n"; // Too many digits
 
 // Large decimal entity, converting from non-ASCII input encoding
 echo "12: " . bin2hex(mb_decode_numericentity(mb_convert_encoding('�', 'UCS-4', 'ASCII'), [0, 0x7FFFFFFF, 0, 0x7FFFFFFF], 'UCS-4')), "\n";
@@ -100,6 +100,8 @@ test("Successive &", "&&#65,", "&A,", [0, 0xFFFF, 0, 0xFFFF], 'ASCII');
 test("Successive &#", "&#2", "&#2", [0, 0xFFFF, 0, 0xFFFF], 'ASCII');
 test("Successive &#x", "&#x2", "&#x2", [0, 0xFFFF, 0, 0xFFFF], 'ASCII');
 
+test("&#x only", "&#x;", "&#x;", [0, 0xFFFF, 0, 0xFFFF], 'ASCII');
+
 // The starting & of an entity can terminate a preceding entity
 test("Successive &#65", "&#65A", "AA", [0, 0xFFFF, 0, 0xFFFF], 'ASCII');
 test("Successive hex entities", "&#x322", "22", [0, 0xFFFF, 0, 0xFFFF], 'ASCII');
@@ -131,6 +133,36 @@ test("Regression test (truncation of successive & with JIS encoding)", "&&&", "&
 // Previously, signed arithmetic was used on convmap entries
 test("Regression test (convmap entries are now treated as unsigned)", "&#7,", "?,", [0x22FFFF11, 0xBF111189, 0x67726511, 0x1161E719], "ASCII");
 
+// Try with '&', '&#', or '&#' at the end of a buffer of wchars, with more input
+// still left to process in the next buffer
+// (mb_decode_numericentity splits its input into 'chunks' and processes it one
+// chunk at a time)
+$convmap = [0, 0xFFFF, 0, 0xFFFF];
+for ($i = 0; $i < 256; $i++) {
+    $padding = str_repeat("a", $i);
+    // First try invalid decimal/hex entities
+    if (mb_decode_numericentity($padding . "&#ZZZ", $convmap, 'UTF-8') !== $padding . "&#ZZZ")
+        die("&#ZZZ is broken when it spans two buffers!");
+    if (mb_decode_numericentity($padding . "&#xZZZ", $convmap, 'UTF-8') !== $padding . "&#xZZZ")
+        die("&#xZZZ is broken when it spans two buffers!");
+    // Now try valid decimal/hex entities
+    if (mb_decode_numericentity($padding . "&#65", $convmap, 'UTF-8') !== $padding . "A")
+        die("&#65 is broken when it spans two buffers!");
+    if (mb_decode_numericentity($padding . "&#x41", $convmap, 'UTF-8') !== $padding . "A")
+        die("&#x41 is broken when it spans two buffers!");
+}
+
+// Try huge entities, big enough to fill an entire buffer
+for ($i = 12; $i < 256; $i++) {
+    $str = "&#" . str_repeat("0", $i) . "65";
+    if (mb_decode_numericentity($str, $convmap, 'UTF-8') !== $str)
+        die("Decimal entity with huge number of digits broken");
+
+    $str = "&#x" . str_repeat("0", $i) . "41";
+    if (mb_decode_numericentity($str, $convmap, 'UTF-8') !== $str)
+        die("Hexadecimal entity with huge number of digits broken");
+}
+
 ?>
 --EXPECT--
 1: ¡¢£¤¥¦§¨©ª«¬­®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ
@@ -142,11 +174,12 @@ test("Regression test (convmap entries are now treated as unsigned)", "&#7,", "?
 7: &#100000000000
 8: &#000000000000
 9: &#00000000000
-10: &#0000000000
-11: &#000000000
-11b: &#x0000000
-11c: &#x00000000
-11d: &#x10000
+10: 00
+11: 00
+11b: 00
+11c: 00
+11d: f0908080
+11e: &#x000000000
 12: 00bc614e
 13: f&ouml;o
 14: mb_decode_numericentity(): Argument #2 ($map) must have a multiple of 4 elements
@@ -164,6 +197,7 @@ Single &: string(1) "&" => string(1) "&" (Good)
 Successive &: string(6) "&&#65," => string(3) "&A," (Good)
 Successive &#: string(8) "&#&#x32;" => string(3) "&#2" (Good)
 Successive &#x: string(9) "&#x&#x32;" => string(4) "&#x2" (Good)
+&#x only: string(4) "&#x;" => string(4) "&#x;" (Good)
 Successive &#65: string(9) "&#65&#65;" => string(2) "AA" (Good)
 Successive hex entities: string(11) "&#x32&#x32;" => string(2) "22" (Good)
 Starting entity immediately after decimal entity which is too long: string(18) "&#10000000000&#65;" => string(14) "&#10000000000A" (Good)