From: Nikita Popov <nikita.ppv@gmail.com>
Date: Thu, 7 May 2020 08:15:57 +0000 (+0200)
Subject: Return false from failed mb_convert_variables()
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5bfa9598f46860b2e42157bd71b79069caa596f2;p=php

Return false from failed mb_convert_variables()

If we fail to detect the encoding return false, just like
mb_convert_encoding() does, and the implementation here clearly
intended. Previously the "pass" pseudo-incoding was returned.
---

diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c
index a529f84809..e105e8638d 100644
--- a/ext/mbstring/mbstring.c
+++ b/ext/mbstring/mbstring.c
@@ -3217,9 +3217,7 @@ PHP_FUNCTION(mb_convert_variables)
 			from_encoding = mbfl_encoding_detector_judge(identd);
 			mbfl_encoding_detector_delete(identd);
 			if (recursion_error) {
-				if (elist != NULL) {
-					efree((void *)elist);
-				}
+				efree(elist);
 				php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
 				RETURN_FALSE;
 			}
@@ -3227,49 +3225,42 @@ PHP_FUNCTION(mb_convert_variables)
 
 		if (!from_encoding) {
 			php_error_docref(NULL, E_WARNING, "Unable to detect encoding");
-			from_encoding = &mbfl_encoding_pass;
+			efree(elist);
+			RETURN_FALSE;
 		}
 	}
-	efree((void *)elist);
-	/* create converter */
-	convd = NULL;
-	if (from_encoding != &mbfl_encoding_pass) {
-		convd = mbfl_buffer_converter_new(from_encoding, to_encoding, 0);
-		/* If this assertion fails this means some memory allocation failure which is a bug */
-		ZEND_ASSERT(convd != NULL);
 
-		mbfl_buffer_converter_illegal_mode(convd, MBSTRG(current_filter_illegal_mode));
-		mbfl_buffer_converter_illegal_substchar(convd, MBSTRG(current_filter_illegal_substchar));
-	}
+	efree(elist);
 
-	/* convert */
-	if (convd != NULL) {
-		n = 0;
-		while (n < argc) {
-			zval *zv = &args[n];
+	convd = mbfl_buffer_converter_new(from_encoding, to_encoding, 0);
+	/* If this assertion fails this means some memory allocation failure which is a bug */
+	ZEND_ASSERT(convd != NULL);
 
-			ZVAL_DEREF(zv);
-			recursion_error = mb_recursive_convert_variable(convd, zv);
-			if (recursion_error) {
-				break;
-			}
-			n++;
-		}
+	mbfl_buffer_converter_illegal_mode(convd, MBSTRG(current_filter_illegal_mode));
+	mbfl_buffer_converter_illegal_substchar(convd, MBSTRG(current_filter_illegal_substchar));
 
-		MBSTRG(illegalchars) += mbfl_buffer_illegalchars(convd);
-		mbfl_buffer_converter_delete(convd);
+	/* convert */
+	n = 0;
+	while (n < argc) {
+		zval *zv = &args[n];
 
+		ZVAL_DEREF(zv);
+		recursion_error = mb_recursive_convert_variable(convd, zv);
 		if (recursion_error) {
-			php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
-			RETURN_FALSE;
+			break;
 		}
+		n++;
 	}
 
-	if (from_encoding) {
-		RETURN_STRING(from_encoding->name);
-	} else {
+	MBSTRG(illegalchars) += mbfl_buffer_illegalchars(convd);
+	mbfl_buffer_converter_delete(convd);
+
+	if (recursion_error) {
+		php_error_docref(NULL, E_WARNING, "Cannot handle recursive references");
 		RETURN_FALSE;
 	}
+
+	RETURN_STRING(from_encoding->name);
 }
 /* }}} */
 
diff --git a/ext/mbstring/tests/mb_convert_encoding_failed_detection.phpt b/ext/mbstring/tests/mb_convert_encoding_failed_detection.phpt
index 961af505ec..b3c8ba10bb 100644
--- a/ext/mbstring/tests/mb_convert_encoding_failed_detection.phpt
+++ b/ext/mbstring/tests/mb_convert_encoding_failed_detection.phpt
@@ -5,7 +5,15 @@ mb_convert_encoding() when encoding detection fails
 
 var_dump(mb_convert_encoding("\xff", "ASCII", ["UTF-8", "UTF-16"]));
 
+$str = "\xff";
+var_dump(mb_convert_variables("ASCII", ["UTF-8", "UTF-16"], $str));
+var_dump(bin2hex($str));
+
 ?>
 --EXPECTF--
 Warning: mb_convert_encoding(): Unable to detect character encoding in %s on line %d
 bool(false)
+
+Warning: mb_convert_variables(): Unable to detect encoding in %s on line %d
+bool(false)
+string(2) "ff"