[Fix] Dont pass invalid keys to ArrayData::remove

Summary: Variant::remove(CVarRef) could end up passing a "null" key to ArrayData::remove if given an invalid key (array or object). In the DEBUG build, this asserted, and in the release build it removed the element with key 0.

The new test case produced incorrect results in the release build, and asserted in the debug build.

Task ID: #768801

Blame Rev:

Reviewers: kma qigao

CC:

Test Plan: fast_tests slow_tests (new test case added).

Revert Plan:

Tags:

Platform Impact (PUBLIC):

Differential Revision: 347370

src/runtime/base/type_variant.cpp

@@ -3478,7 +3478,9 @@ void Variant::removeImpl(CVarRef key, bool isString /* false */) {
         if (isString) {
           escalated = arr->remove(key, (arr->getCount() > 1));
         } else {
-          escalated = arr->remove(key.toKey(), (arr->getCount() > 1));
+          const VarNR &k = key.toKey();
+          if (k.isNull()) return;
+          escalated = arr->remove(k, (arr->getCount() > 1));
         }
         if (escalated) {
           set(escalated);

src/test/test_code_run.cpp

@@ -7242,6 +7242,13 @@ bool TestCodeRun::TestUnset() {
        "goo();\n"
        "unset($this);\n"
        "var_dump($this);\n");
+
+  MVCR("<?php "
+       "function rmv($a, $b) { unset($a[$b]); return $a; }"
+       "$a = array('foo');"
+       "$b = array();"
+       "var_dump(rmv($a, $b));");
+
   return true;
 }