OSDN Git Service

Bailout from compilation if an invoke is malformed.
authorNicolas Geoffray <ngeoffray@google.com>
Thu, 18 Jun 2015 10:11:27 +0000 (11:11 +0100)
committerNicolas Geoffray <ngeoffray@google.com>
Thu, 18 Jun 2015 11:47:06 +0000 (12:47 +0100)
Because the verifier does not check trivially dead instructions,
the compilers must prepare for bogus instructions. This change
fixes the case the arguments for an invoke do not match the
formal parameters.

bug:21865459

Change-Id: Ie9fa9dbaedaad0350a52391252e18c045056454d

compiler/optimizing/builder.cc
compiler/optimizing/optimizing_compiler_stats.h
test/503-dead-instructions/expected.txt [new file with mode: 0644]
test/503-dead-instructions/info.txt [new file with mode: 0644]
test/503-dead-instructions/smali/DeadInstructions.smali [new file with mode: 0644]
test/503-dead-instructions/src/Main.java [new file with mode: 0644]

index cdd7636..793dd28 100644 (file)
@@ -603,7 +603,12 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
   const char* descriptor = dex_file_->StringDataByIdx(proto_id.shorty_idx_);
   Primitive::Type return_type = Primitive::GetType(descriptor[0]);
   bool is_instance_call = invoke_type != kStatic;
-  size_t number_of_arguments = strlen(descriptor) - (is_instance_call ? 0 : 1);
+  // Remove the return type from the 'proto'.
+  size_t number_of_arguments = strlen(descriptor) - 1;
+  if (is_instance_call) {
+    // One extra argument for 'this'.
+    ++number_of_arguments;
+  }
 
   MethodReference target_method(dex_file_, method_idx);
   uintptr_t direct_code;
@@ -614,7 +619,8 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
   if (!compiler_driver_->ComputeInvokeInfo(dex_compilation_unit_, dex_pc, true, true,
                                            &optimized_invoke_type, &target_method, &table_index,
                                            &direct_code, &direct_method)) {
-    VLOG(compiler) << "Did not compile " << PrettyMethod(method_idx, *dex_file_)
+    VLOG(compiler) << "Did not compile "
+                   << PrettyMethod(dex_compilation_unit_->GetDexMethodIndex(), *dex_file_)
                    << " because a method call could not be resolved";
     MaybeRecordStat(MethodCompilationStat::kNotCompiledUnresolvedMethod);
     return false;
@@ -746,26 +752,45 @@ bool HGraphBuilder::BuildInvoke(const Instruction& instruction,
     start_index = 1;
   }
 
-  uint32_t descriptor_index = 1;
+  uint32_t descriptor_index = 1;  // Skip the return type.
   uint32_t argument_index = start_index;
   if (is_string_init) {
     start_index = 1;
   }
-  for (size_t i = start_index; i < number_of_vreg_arguments; i++, argument_index++) {
+  for (size_t i = start_index;
+       // Make sure we don't go over the expected arguments or over the number of
+       // dex registers given. If the instruction was seen as dead by the verifier,
+       // it hasn't been properly checked.
+       (i < number_of_vreg_arguments) && (argument_index < number_of_arguments);
+       i++, argument_index++) {
     Primitive::Type type = Primitive::GetType(descriptor[descriptor_index++]);
     bool is_wide = (type == Primitive::kPrimLong) || (type == Primitive::kPrimDouble);
-    // Longs and doubles should be in pairs, that is, sequential registers. The verifier should
-    // reject any class where this is violated.
-    DCHECK(is_range || !is_wide || (args[i] + 1 == args[i + 1]))
-        << "Non sequential register pair in " << dex_compilation_unit_->GetSymbol()
-        << " at " << dex_pc;
+    if (!is_range
+        && is_wide
+        && ((i + 1 == number_of_vreg_arguments) || (args[i] + 1 != args[i + 1]))) {
+      // Longs and doubles should be in pairs, that is, sequential registers. The verifier should
+      // reject any class where this is violated. However, the verifier only does these checks
+      // on non trivially dead instructions, so we just bailout the compilation.
+      VLOG(compiler) << "Did not compile "
+                     << PrettyMethod(dex_compilation_unit_->GetDexMethodIndex(), *dex_file_)
+                     << " because of non-sequential dex register pair in wide argument";
+      MaybeRecordStat(MethodCompilationStat::kNotCompiledMalformedOpcode);
+      return false;
+    }
     HInstruction* arg = LoadLocal(is_range ? register_index + i : args[i], type);
     invoke->SetArgumentAt(argument_index, arg);
     if (is_wide) {
       i++;
     }
   }
-  DCHECK_EQ(argument_index, number_of_arguments);
+
+  if (argument_index != number_of_arguments) {
+    VLOG(compiler) << "Did not compile "
+                   << PrettyMethod(dex_compilation_unit_->GetDexMethodIndex(), *dex_file_)
+                   << " because of wrong number of arguments in invoke instruction";
+    MaybeRecordStat(MethodCompilationStat::kNotCompiledMalformedOpcode);
+    return false;
+  }
 
   if (invoke->IsInvokeStaticOrDirect()) {
     invoke->SetArgumentAt(argument_index, graph_->GetCurrentMethod());
index b988813..53d052b 100644 (file)
@@ -38,6 +38,7 @@ enum MethodCompilationStat {
   kNotCompiledClassNotVerified,
   kNotCompiledHugeMethod,
   kNotCompiledLargeMethodNoBranches,
+  kNotCompiledMalformedOpcode,
   kNotCompiledNoCodegen,
   kNotCompiledPathological,
   kNotCompiledSpaceFilter,
@@ -106,6 +107,7 @@ class OptimizingCompilerStats {
       case kNotCompiledClassNotVerified : return "kNotCompiledClassNotVerified";
       case kNotCompiledHugeMethod : return "kNotCompiledHugeMethod";
       case kNotCompiledLargeMethodNoBranches : return "kNotCompiledLargeMethodNoBranches";
+      case kNotCompiledMalformedOpcode : return "kNotCompiledMalformedOpcode";
       case kNotCompiledNoCodegen : return "kNotCompiledNoCodegen";
       case kNotCompiledPathological : return "kNotCompiledPathological";
       case kNotCompiledSpaceFilter : return "kNotCompiledSpaceFilter";
diff --git a/test/503-dead-instructions/expected.txt b/test/503-dead-instructions/expected.txt
new file mode 100644 (file)
index 0000000..ccaf6f8
--- /dev/null
@@ -0,0 +1 @@
+Enter
diff --git a/test/503-dead-instructions/info.txt b/test/503-dead-instructions/info.txt
new file mode 100644 (file)
index 0000000..7e3f1ab
--- /dev/null
@@ -0,0 +1,2 @@
+Regression test for the building phase of the optimizing
+compiler. See comment in smali file.
diff --git a/test/503-dead-instructions/smali/DeadInstructions.smali b/test/503-dead-instructions/smali/DeadInstructions.smali
new file mode 100644 (file)
index 0000000..9f6c565
--- /dev/null
@@ -0,0 +1,63 @@
+# Copyright (C) 2015 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+.class public LDeadInstructions;
+
+.super Ljava/lang/Object;
+
+.method public static method1()V
+   .registers 2
+   return-void
+   # Create a label and a branch to that label to trick the
+   # optimizing compiler into thinking the invoke is live.
+   :start
+   const/4 v0, 0
+   const/4 v1, 0
+   # Provide more arguments than we should. Because this is dead
+   # code, the verifier will not check the argument count. So
+   # the compilers must do the same.
+   invoke-static {v0, v1}, LDeadInstructions;->method1()V
+   goto :start
+.end method
+
+.method public static method2(J)V
+   .registers 3
+   return-void
+   :start
+   const/4 v0, 0
+   const/4 v1, 0
+   const/4 v2, 0
+   # Give a non-sequential pair for the long argument.
+   invoke-static {v0, v2}, LDeadInstructions;->method2(J)V
+   goto :start
+.end method
+
+.method public static method3()V
+   .registers 1
+   return-void
+   :start
+   const/4 v0, 0
+   # Give one half of a pair.
+   invoke-static {v0}, LDeadInstructions;->method2(J)V
+   goto :start
+.end method
+
+.method public static method4()V
+   .registers 2
+   return-void
+   :start
+   # Provide less arguments than we should.
+   invoke-static {}, LDeadInstructions;->method3(J)V
+   goto :start
+.end method
diff --git a/test/503-dead-instructions/src/Main.java b/test/503-dead-instructions/src/Main.java
new file mode 100644 (file)
index 0000000..6249dc7
--- /dev/null
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.lang.reflect.Method;
+
+public class Main {
+  public static void main(String[] args) throws Exception {
+    // Workaround for b/18051191.
+    System.out.println("Enter");
+    Class<?> c = Class.forName("DeadInstructions");
+    Method m = c.getMethod("method1");
+    Object[] arguments1 = { };
+    m.invoke(null, arguments1);
+
+    Object[] arguments2 = { (long)4 };
+    m = c.getMethod("method2", long.class);
+    m.invoke(null, arguments2);
+
+    Object[] arguments3 = { };
+    m = c.getMethod("method3");
+    m.invoke(null, arguments3);
+
+    Object[] arguments4 = { };
+    m = c.getMethod("method4");
+    m.invoke(null, arguments4);
+  }
+}