From: Nikita Popov <nikita.ppv@gmail.com>
Date: Mon, 28 Sep 2020 11:11:07 +0000 (+0200)
Subject: Consistently handle out of bounds offsets in iconv_strpos()
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=42168304b58ab939634a99a678ddd56eada46e39;p=php

Consistently handle out of bounds offsets in iconv_strpos()

Same as in all other strpos() style functions, throw ValueError on
out of bounds offset.
---

diff --git a/ext/iconv/iconv.c b/ext/iconv/iconv.c
index 89f2ffb37f..84f24baabb 100644
--- a/ext/iconv/iconv.c
+++ b/ext/iconv/iconv.c
@@ -884,6 +884,10 @@ static php_iconv_err_t _php_iconv_strpos(size_t *pretval,
 
 	iconv_close(cd);
 
+	if (err == PHP_ICONV_ERR_SUCCESS && offset > cnt) {
+		return PHP_ICONV_ERR_OUT_BY_BOUNDS;
+	}
+
 	return err;
 }
 /* }}} */
@@ -1764,6 +1768,10 @@ static void _php_iconv_show_error(php_iconv_err_t err, const char *out_charset,
 			php_error_docref(NULL, E_WARNING, "Malformed string");
 			break;
 
+		case PHP_ICONV_ERR_OUT_BY_BOUNDS:
+			zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
+			break;
+
 		default:
 			/* other error */
 			php_error_docref(NULL, E_NOTICE, "Unknown error (%d)", errno);
@@ -1881,12 +1889,13 @@ PHP_FUNCTION(iconv_strpos)
 		}
 		offset += haystk_len;
 		if (offset < 0) { /* If offset before start */
-			php_error_docref(NULL, E_WARNING, "Offset not contained in string.");
-			RETURN_FALSE;
+			zend_argument_value_error(3, "must be contained in argument #1 ($haystack)");
+			RETURN_THROWS();
 		}
 	}
 
 	if (ZSTR_LEN(ndl) < 1) {
+		// TODO: Support empty needles!
 		RETURN_FALSE;
 	}
 
diff --git a/ext/iconv/php_iconv.h b/ext/iconv/php_iconv.h
index 89506fab53..901ba440fd 100644
--- a/ext/iconv/php_iconv.h
+++ b/ext/iconv/php_iconv.h
@@ -87,7 +87,8 @@ typedef enum _php_iconv_err_t {
 	PHP_ICONV_ERR_ILLEGAL_CHAR      = 5,
 	PHP_ICONV_ERR_UNKNOWN           = 6,
 	PHP_ICONV_ERR_MALFORMED         = 7,
-	PHP_ICONV_ERR_ALLOC             = 8
+	PHP_ICONV_ERR_ALLOC             = 8,
+	PHP_ICONV_ERR_OUT_BY_BOUNDS     = 9
 } php_iconv_err_t;
 /* }}} */
 
diff --git a/ext/iconv/tests/iconv_strpos.phpt b/ext/iconv/tests/iconv_strpos.phpt
index 1dafd6c06b..df9427d2c6 100644
--- a/ext/iconv/tests/iconv_strpos.phpt
+++ b/ext/iconv/tests/iconv_strpos.phpt
@@ -16,14 +16,19 @@ function foo($haystk, $needle, $offset, $to_charset = false, $from_charset = fal
     } catch (ValueError $exception) {
         echo $exception->getMessage() . "\n";
     }
-    if ($to_charset !== false) {
-        var_dump(iconv_strpos($haystk, $needle, $offset, $to_charset));
-    } else {
-        var_dump(iconv_strpos($haystk, $needle, $offset));
+    try {
+        if ($to_charset !== false) {
+            var_dump(iconv_strpos($haystk, $needle, $offset, $to_charset));
+        } else {
+            var_dump(iconv_strpos($haystk, $needle, $offset));
+        }
+    } catch (ValueError $exception) {
+        echo $exception->getMessage() . "\n";
     }
 }
 foo("abecdbcdabef", "bcd", -1);
 foo("abecdbcdabef", "bcd", -7);
+foo("abecdbcdabef", "bcd", 12);
 foo("abecdbcdabef", "bcd", 100000);
 foo("abcabcabcdabcababcdabc", "bcd", 0);
 foo("abcabcabcdabcababcdabc", "bcd", 10);
@@ -41,8 +46,10 @@ bool(false)
 bool(false)
 int(5)
 int(5)
-strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 bool(false)
+bool(false)
+strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 int(7)
 int(7)
 int(16)
diff --git a/ext/iconv/tests/iconv_strpos_variation5.phpt b/ext/iconv/tests/iconv_strpos_variation5.phpt
index 4feef545a4..625c44efa7 100644
--- a/ext/iconv/tests/iconv_strpos_variation5.phpt
+++ b/ext/iconv/tests/iconv_strpos_variation5.phpt
@@ -32,25 +32,29 @@ $needle_mb = base64_decode('44CC');
 for ($i = -30; $i <= 60; $i += 10) {
     echo "\n**-- Offset is: $i --**\n";
     echo "-- ASCII String --\n";
-    var_dump(iconv_strpos($string_ascii, $needle_ascii, $i));
+    try {
+        var_dump(iconv_strpos($string_ascii, $needle_ascii, $i));
+    } catch (ValueError $e) {
+        echo $e->getMessage(), "\n";
+    }
     echo "--Multibyte String --\n";
-    var_dump(iconv_strpos($string_mb, $needle_mb, $i, 'UTF-8'));
+    try {
+        var_dump(iconv_strpos($string_mb, $needle_mb, $i, 'UTF-8'));
+    } catch (ValueError $e) {
+        echo $e->getMessage(), "\n";
+    }
 }
 
 echo "Done";
 ?>
---EXPECTF--
+--EXPECT--
 *** Testing iconv_strpos() : usage variations ***
 
 **-- Offset is: -30 --**
 -- ASCII String --
-
-Warning: iconv_strpos(): Offset not contained in string. in %s on line %d
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 --Multibyte String --
-
-Warning: iconv_strpos(): Offset not contained in string. in %s on line %d
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 
 **-- Offset is: -20 --**
 -- ASCII String --
@@ -84,25 +88,25 @@ int(20)
 
 **-- Offset is: 30 --**
 -- ASCII String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 --Multibyte String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 
 **-- Offset is: 40 --**
 -- ASCII String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 --Multibyte String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 
 **-- Offset is: 50 --**
 -- ASCII String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 --Multibyte String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 
 **-- Offset is: 60 --**
 -- ASCII String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 --Multibyte String --
-bool(false)
+iconv_strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack)
 Done