Fixed bug #63217

Don't automatically convert literal string keys to integers on array access, as we may be dealing with an ArrayAccess object, rather than a plain array.
php · Jul 2, 2018 · 30156d588c07e26d4e752ddb62344e96854d4773 · 30156d5
1 parent 6d61814
commit 30156d588c07e26d4e752ddb62344e96854d4773
diff --git a/NEWS b/NEWS
@@ -14,6 +14,8 @@ PHP                                                                        NEWS
   . Fixed bug #76439 (Changed behaviour in unclosed HereDoc). (Nikita, tpunt)
   . Added syslog.facility and syslog.ident INI entries for customizing syslog
     logging. (Philip Prindeville)
+  . Fixed bug #63217 (Constant numeric strings become integers when used as
+    ArrayAccess offset). (Rudi Theunissen)
 
 - DOM:
   . Fixed bug #76285 (DOMDocument::formatOutput attribute sometimes ignored).

diff --git a/UPGRADING b/UPGRADING
@@ -44,6 +44,12 @@ Core:
     the following "FOO;" will cause a syntax error. This issue can always be
     resolved by choosing an ending label that does not occur within the contents
     of the string.
+  . Array accesses of type $obj["123"], where $obj implements ArrayAccess and
+    "123" is an integral string literal will no longer result in an implicit
+    conversion to integer, i.e., $obj->offsetGet("123") will be called instead
+    of $obj->offsetGet(123). This matches existing behavior for non-literals.
+    The behavior of arrays is not affected in any way, they continue to
+    implicitly convert integeral string keys to integers.
   . In PHP, static properties are shared between inheriting classes, unless the
     static property is explicitly overridden in a child class. However, due to
     an implementation artifact it was possible to separate the static properties

diff --git a/Zend/tests/bug29883.phpt b/Zend/tests/bug29883.phpt
@@ -4,10 +4,18 @@ Bug #29883 (isset gives invalid values on strings)
 <?php
 $x = "bug";
 var_dump(isset($x[-10]));
+var_dump(isset($x[1]));
 var_dump(isset($x["1"]));
-echo $x["1"]."\n";
+var_dump($x[-10])."\n";
+var_dump($x[1])."\n";
+var_dump($x["1"])."\n";
 ?>
---EXPECT--
+--EXPECTF--
 bool(false)
 bool(true)
-u
+bool(true)
+
+Notice: Uninitialized string offset: -10 in %s on line 6
+string(0) ""
+string(1) "u"
+string(1) "u"
diff --git a/Zend/tests/bug55135.phpt b/Zend/tests/bug55135.phpt
@@ -19,7 +19,7 @@ unset($array[1]);
 print_r($array);
 
 $array = array(1 => 2);
-$a = 1;
+
 unset($array["1"]);
 print_r($array);
 ?>

diff --git a/Zend/tests/bug63217.phpt b/Zend/tests/bug63217.phpt
@@ -0,0 +1,123 @@
+--TEST--
+Bug #63217 (Constant numeric strings become integers when used as ArrayAccess offset)
+--INI--
+opcache.enable_cli=1
+opcache.enable=1
+opcache.optimization_level=-1
+--FILE--
+<?php
+class Test implements ArrayAccess {
+    public function offsetExists($offset) {
+        echo "offsetExists given ";
+        var_dump($offset);
+    }
+    public function offsetUnset($offset) {
+        echo "offsetUnset given ";
+        var_dump($offset);
+    }
+    public function offsetSet($offset, $value) {
+        echo "offsetSet given ";
+        var_dump($offset);
+    }
+    public function offsetGet($offset) {
+        echo "offsetGet given ";
+        var_dump($offset);
+    }
+}
+
+$test = new Test;
+
+/* These should all produce string(...) "..." output and not int(...) */
+isset($test['0']);
+isset($test['123']);
+unset($test['0']);
+unset($test['123']);
+$test['0'] = true;
+$test['123'] = true;
+$foo = $test['0'];
+$foo = $test['123'];
+
+/* These caused the same bug, but in opcache rather than the compiler */
+isset($test[(string)'0']);
+isset($test[(string)'123']);
+unset($test[(string)'0']);
+unset($test[(string)'123']);
+$test[(string)'0'] = true;
+$test[(string)'123'] = true;
+$foo = $test[(string)'0'];
+$foo = $test[(string)'123'];
+
+/**
+ * @see https://github.com/php/php-src/pull/2607#issuecomment-313781748
+ */
+function test(): string {
+    $array["10"] = 42;
+    foreach ($array as $key => $value) {
+        return $key;
+    }
+}
+
+var_dump(test());
+
+/**
+ * Make sure we don't break arrays.
+ */
+$array = [];
+
+$key = '123';
+
+$array[$key] = 1;
+$array['321'] = 2;
+$array['abc'] = 3;
+
+var_dump($array);
+
+/**
+ * Make sure that we haven't broken ArrayObject
+ */
+$ao = new ArrayObject();
+
+$key = '123';
+
+$ao = [];
+$ao[$key] = 1;
+$ao['321'] = 2;
+$ao['abc'] = 3;
+
+var_dump($ao);
+
+?>
+--EXPECT--
+offsetExists given string(1) "0"
+offsetExists given string(3) "123"
+offsetUnset given string(1) "0"
+offsetUnset given string(3) "123"
+offsetSet given string(1) "0"
+offsetSet given string(3) "123"
+offsetGet given string(1) "0"
+offsetGet given string(3) "123"
+offsetExists given string(1) "0"
+offsetExists given string(3) "123"
+offsetUnset given string(1) "0"
+offsetUnset given string(3) "123"
+offsetSet given string(1) "0"
+offsetSet given string(3) "123"
+offsetGet given string(1) "0"
+offsetGet given string(3) "123"
+string(2) "10"
+array(3) {
+  [123]=>
+  int(1)
+  [321]=>
+  int(2)
+  ["abc"]=>
+  int(3)
+}
+array(3) {
+  [123]=>
+  int(1)
+  [321]=>
+  int(2)
+  ["abc"]=>
+  int(3)
+}
diff --git a/Zend/tests/empty_str_offset.phpt b/Zend/tests/empty_str_offset.phpt
@@ -17,16 +17,24 @@ var_dump(empty($str[5])); // 1
 var_dump(empty($str[8]));
 var_dump(empty($str[10000]));
 // non-numeric offsets
-print "- string ---\n";
-var_dump(empty($str['-1']));
+print "- string literal ---\n";
+var_dump(empty($str['-1'])); // 3
 var_dump(empty($str['-10']));
-var_dump(empty($str['-4'])); // 0
 var_dump(empty($str['0']));
 var_dump(empty($str['1']));
 var_dump(empty($str['4'])); // 0
 var_dump(empty($str['1.5']));
 var_dump(empty($str['good']));
 var_dump(empty($str['3 and a half']));
+print "- string variable ---\n";
+var_dump(empty($str[$key = '-1'])); // 3
+var_dump(empty($str[$key = '-10']));
+var_dump(empty($str[$key = '0']));
+var_dump(empty($str[$key = '1']));
+var_dump(empty($str[$key = '4'])); // 0
+var_dump(empty($str[$key = '1.5']));
+var_dump(empty($str[$key = 'good']));
+var_dump(empty($str[$key = '3 and a half']));
 print "- bool ---\n";
 var_dump(empty($str[true]));
 var_dump(empty($str[false]));
@@ -54,7 +62,7 @@ var_dump(empty($str[$f]));
 print "done\n";
 
 ?>
---EXPECT--
+--EXPECTF--
 - empty ---
 bool(false)
 bool(true)
@@ -65,9 +73,17 @@ bool(true)
 bool(false)
 bool(true)
 bool(true)
-- string ---
+- string literal ---
 bool(false)
 bool(true)
+bool(false)
+bool(false)
+bool(true)
+bool(true)
+bool(true)
+bool(true)
+- string variable ---
+bool(false)
 bool(true)
 bool(false)
 bool(false)

diff --git a/Zend/tests/isset_str_offset.phpt b/Zend/tests/isset_str_offset.phpt
@@ -16,15 +16,24 @@ var_dump(isset($str[5])); // 1
 var_dump(isset($str[8]));
 var_dump(isset($str[10000]));
 // non-numeric offsets
-print "- string ---\n";
-var_dump(isset($str['-1']));
+print "- string literal ---\n";
+var_dump(isset($str['-1'])); // 3
 var_dump(isset($str['-10']));
 var_dump(isset($str['0']));
 var_dump(isset($str['1']));
 var_dump(isset($str['4'])); // 0
 var_dump(isset($str['1.5']));
 var_dump(isset($str['good']));
 var_dump(isset($str['3 and a half']));
+print "- string variable ---\n";
+var_dump(isset($str[$key = '-1'])); // 3
+var_dump(isset($str[$key = '-10']));
+var_dump(isset($str[$key = '0']));
+var_dump(isset($str[$key = '1']));
+var_dump(isset($str[$key = '4'])); // 0
+var_dump(isset($str[$key = '1.5']));
+var_dump(isset($str[$key = 'good']));
+var_dump(isset($str[$key = '3 and a half']));
 print "- bool ---\n";
 var_dump(isset($str[true]));
 var_dump(isset($str[false]));
@@ -51,7 +60,7 @@ var_dump(isset($str[$f]));
 print "done\n";
 
 ?>
---EXPECT--
+--EXPECTF--
 - isset ---
 bool(true)
 bool(false)
@@ -61,7 +70,16 @@ bool(true)
 bool(true)
 bool(false)
 bool(false)
-- string ---
+- string literal ---
+bool(true)
+bool(false)
+bool(true)
+bool(true)
+bool(true)
+bool(false)
+bool(false)
+bool(false)
+- string variable ---
 bool(true)
 bool(false)
 bool(true)

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
@@ -2672,7 +2672,6 @@ static zend_op *zend_delayed_compile_dim(znode *result, zend_ast *ast, uint32_t
 		dim_node.op_type = IS_UNUSED;
 	} else {
 		zend_compile_expr(&dim_node, dim_ast);
-		zend_handle_numeric_op(&dim_node);
 	}
 
 	opline = zend_delayed_emit_op(result, ZEND_FETCH_DIM_R, &var_node, &dim_node);

diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c
@@ -1731,10 +1731,8 @@ static zend_always_inline zval *zend_fetch_dimension_address_inner(HashTable *ht
 		}
 	} else if (EXPECTED(Z_TYPE_P(dim) == IS_STRING)) {
 		offset_key = Z_STR_P(dim);
-		if (dim_type != IS_CONST) {
-			if (ZEND_HANDLE_NUMERIC(offset_key, hval)) {
-				goto num_index;
-			}
+		if (ZEND_HANDLE_NUMERIC(offset_key, hval)) {
+			goto num_index;
 		}
 str_index:
 		retval = zend_hash_find_ex(ht, offset_key, dim_type == IS_CONST);

diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
@@ -5538,10 +5538,8 @@ ZEND_VM_C_LABEL(unset_dim_array):
 ZEND_VM_C_LABEL(offset_again):
 			if (EXPECTED(Z_TYPE_P(offset) == IS_STRING)) {
 				key = Z_STR_P(offset);
-				if (OP2_TYPE != IS_CONST) {
-					if (ZEND_HANDLE_NUMERIC(key, hval)) {
-						ZEND_VM_C_GOTO(num_index_dim);
-					}
+				if (ZEND_HANDLE_NUMERIC(key, hval)) {
+					ZEND_VM_C_GOTO(num_index_dim);
 				}
 ZEND_VM_C_LABEL(str_index_dim):
 				if (ht == &EG(symbol_table)) {
@@ -6286,10 +6284,8 @@ ZEND_VM_C_LABEL(isset_dim_obj_array):
 ZEND_VM_C_LABEL(isset_again):
 		if (EXPECTED(Z_TYPE_P(offset) == IS_STRING)) {
 			str = Z_STR_P(offset);
-			if (OP2_TYPE != IS_CONST) {
-				if (ZEND_HANDLE_NUMERIC(str, hval)) {
-					ZEND_VM_C_GOTO(num_index_prop);
-				}
+			if (ZEND_HANDLE_NUMERIC(str, hval)) {
+				ZEND_VM_C_GOTO(num_index_prop);
 			}
 			value = zend_hash_find_ex_ind(ht, str, OP2_TYPE == IS_CONST);
 		} else if (EXPECTED(Z_TYPE_P(offset) == IS_LONG)) {