Fix verifier checks on interface methods.

We were disallowing interfaces in the IsInheritedMethod even though
the function can be called with them. This could cause some failing
DCHECKS if the verifier cannot find methods in some situations.

We also fixed a small issue in the verifier where we allowed
non-public java.lang.Object methods to be considered valid for
interface dispatch.

Test: ./test.py --host -j50
Test: Compile an app with bad bytecodes (See bug)
Bug: 64158483
Bug: 64274113
Change-Id: Ia79f25be0001efc4069a411a0b34476bd0871803

13 files changed, 232 insertions, 2 deletions

diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index 6642869900..c775cf4613 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -464,14 +464,25 @@ ArtMethod* Class::FindInterfaceMethod(ObjPtr<DexCache> dex_cache,
   return FindInterfaceMethod(name, signature, pointer_size);
 }
 
+static inline bool IsValidInheritanceCheck(ObjPtr<mirror::Class> klass,
+                                           ObjPtr<mirror::Class> declaring_class)
+    REQUIRES_SHARED(Locks::mutator_lock_) {
+  if (klass->IsArrayClass()) {
+    return declaring_class->IsObjectClass();
+  } else if (klass->IsInterface()) {
+    return declaring_class->IsObjectClass() || declaring_class == klass;
+  } else {
+    return klass->IsSubClass(declaring_class);
+  }
+}
+
 static inline bool IsInheritedMethod(ObjPtr<mirror::Class> klass,
                                      ObjPtr<mirror::Class> declaring_class,
                                      ArtMethod& method)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   DCHECK_EQ(declaring_class, method.GetDeclaringClass());
   DCHECK_NE(klass, declaring_class);
-  DCHECK(klass->IsArrayClass() ? declaring_class->IsObjectClass()
-                               : klass->IsSubClass(declaring_class));
+  DCHECK(IsValidInheritanceCheck(klass, declaring_class));
   uint32_t access_flags = method.GetAccessFlags();
   if ((access_flags & (kAccPublic | kAccProtected)) != 0) {
     return true;
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 0c460db7da..312d8dfc18 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -3861,10 +3861,20 @@ ArtMethod* MethodVerifier::ResolveMethodAndCheckAccess(
   // TODO: Maybe we should not record dependency if the invoke type does not match the lookup type.
   VerifierDeps::MaybeRecordMethodResolution(*dex_file_, dex_method_idx, res_method);
 
+  bool must_fail = false;
+  // This is traditional and helps with screwy bytecode. It will tell you that, yes, a method
+  // exists, but that it's called incorrectly. This significantly helps debugging, as locally it's
+  // hard to see the differences.
+  // If we don't have res_method here we must fail. Just use this bool to make sure of that with a
+  // DCHECK.
   if (res_method == nullptr) {
+    must_fail = true;
     // Try to find the method also with the other type for better error reporting below
     // but do not store such bogus lookup result in the DexCache or VerifierDeps.
     if (klass->IsInterface()) {
+      // NB This is normally not really allowed but we want to get any static or private object
+      // methods for error message purposes. This will never be returned.
+      // TODO We might want to change the verifier to not require this.
       res_method = klass->FindClassMethod(dex_cache_.Get(), dex_method_idx, pointer_size);
     } else {
       // If there was an interface method with the same signature,
@@ -3922,6 +3932,20 @@ ArtMethod* MethodVerifier::ResolveMethodAndCheckAccess(
     }
   }
 
+  // Check specifically for non-public object methods being provided for interface dispatch. This
+  // can occur if we failed to find a method with FindInterfaceMethod but later find one with
+  // FindClassMethod for error message use.
+  if (method_type == METHOD_INTERFACE &&
+      res_method->GetDeclaringClass()->IsObjectClass() &&
+      !res_method->IsPublic()) {
+    Fail(VERIFY_ERROR_NO_METHOD) << "invoke-interface " << klass->PrettyDescriptor() << "."
+                                 << dex_file_->GetMethodName(method_id) << " "
+                                 << dex_file_->GetMethodSignature(method_id) << " resolved to "
+                                 << "non-public object method " << res_method->PrettyMethod() << " "
+                                 << "but non-public Object methods are excluded from interface "
+                                 << "method resolution.";
+    return nullptr;
+  }
   // Check if access is allowed.
   if (!referrer.CanAccessMember(res_method->GetDeclaringClass(), res_method->GetAccessFlags())) {
     Fail(VERIFY_ERROR_ACCESS_METHOD) << "illegal method access (call "
@@ -3950,6 +3974,13 @@ ArtMethod* MethodVerifier::ResolveMethodAndCheckAccess(
                                        "type of " << res_method->PrettyMethod();
     return nullptr;
   }
+  // Make sure we weren't expecting to fail.
+  DCHECK(!must_fail) << "invoke type (" << method_type << ")"
+                     << klass->PrettyDescriptor() << "."
+                     << dex_file_->GetMethodName(method_id) << " "
+                     << dex_file_->GetMethodSignature(method_id) << " unexpectedly resolved to "
+                     << res_method->PrettyMethod() << " without error. Initially this method was "
+                     << "not found so we were expecting to fail for some reason.";
   return res_method;
 }
 
diff --git a/test/075-verification-error/expected.txt b/test/075-verification-error/expected.txt
index 6e4f584d3a..7ccc32cbac 100644
--- a/test/075-verification-error/expected.txt
+++ b/test/075-verification-error/expected.txt
@@ -10,3 +10,4 @@ Got expected IllegalAccessError (smethod)
 Got expected IllegalAccessError (meth-class)
 Got expected IllegalAccessError (field-class)
 Got expected IllegalAccessError (meth-meth)
+Got expected IncompatibleClassChangeError (interface)
diff --git a/test/075-verification-error/src/BadIfaceImpl.java b/test/075-verification-error/src/BadIfaceImpl.java
new file mode 100644
index 0000000000..fa2a970127
--- /dev/null
+++ b/test/075-verification-error/src/BadIfaceImpl.java
@@ -0,0 +1,17 @@
+/*
+ * Copyright (C) 2017 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.
+ */
+
+public class BadIfaceImpl implements BadInterface { }
diff --git a/test/075-verification-error/src/BadInterface.java b/test/075-verification-error/src/BadInterface.java
new file mode 100644
index 0000000000..439aba467a
--- /dev/null
+++ b/test/075-verification-error/src/BadInterface.java
@@ -0,0 +1,21 @@
+/*
+ * Copyright (C) 2017 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.
+ */
+
+public interface BadInterface {
+  public default Object internalClone() {
+    throw new Error("Should not be called");
+  }
+}
diff --git a/test/075-verification-error/src/Main.java b/test/075-verification-error/src/Main.java
index 3f2881eb10..13aeaee7e4 100644
--- a/test/075-verification-error/src/Main.java
+++ b/test/075-verification-error/src/Main.java
@@ -28,6 +28,20 @@ public class Main {
         testClassNewInstance();
         testMissingStuff();
         testBadAccess();
+        testBadInterfaceMethod();
+     }
+    /**
+     * Try to create and invoke a non-existant interface method.
+     */
+    static void testBadInterfaceMethod() {
+        BadInterface badiface = new BadIfaceImpl();
+        try {
+            badiface.internalClone();
+        } catch (IncompatibleClassChangeError icce) {
+            // TODO b/64274113 This should really be an NSME
+            System.out.println("Got expected IncompatibleClassChangeError (interface)");
+            if (VERBOSE) System.out.println("--- " + icce);
+        }
     }
 
     /**
diff --git a/test/075-verification-error/src2/BadInterface.java b/test/075-verification-error/src2/BadInterface.java
new file mode 100644
index 0000000000..5d939cb264
--- /dev/null
+++ b/test/075-verification-error/src2/BadInterface.java
@@ -0,0 +1,17 @@
+/*
+ * Copyright (C) 2017 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.
+ */
+public interface BadInterface { }
+
diff --git a/test/162-method-resolution/expected.txt b/test/162-method-resolution/expected.txt
index 1bf39c90de..9b48a4cc8f 100644
--- a/test/162-method-resolution/expected.txt
+++ b/test/162-method-resolution/expected.txt
@@ -41,3 +41,6 @@ Test9Derived.foo()
 Calling Test9User2.test():
 Caught java.lang.reflect.InvocationTargetException
   caused by java.lang.IncompatibleClassChangeError
+Calling Test10User.test():
+Caught java.lang.reflect.InvocationTargetException
+  caused by java.lang.IncompatibleClassChangeError
diff --git a/test/162-method-resolution/jasmin/Test10Base.j b/test/162-method-resolution/jasmin/Test10Base.j
new file mode 100644
index 0000000000..628f38d6d5
--- /dev/null
+++ b/test/162-method-resolution/jasmin/Test10Base.j
@@ -0,0 +1,25 @@
+; Copyright (C) 2017 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 Test10Base
+.super                   java/lang/Object
+.implements              Test10Interface
+
+.method                  public <init>()V
+   .limit stack          1
+   .limit locals         1
+   aload_0
+   invokespecial         java/lang/Object/<init>()V
+   return
+.end method
diff --git a/test/162-method-resolution/jasmin/Test10User.j b/test/162-method-resolution/jasmin/Test10User.j
new file mode 100644
index 0000000000..6beadaba12
--- /dev/null
+++ b/test/162-method-resolution/jasmin/Test10User.j
@@ -0,0 +1,36 @@
+; Copyright (C) 2017 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 Test10User
+.super java/lang/Object
+
+.method public static test()V
+    .limit stack 3
+    .limit locals 3
+    new Test10Base
+    dup
+    invokespecial Test10Base.<init>()V
+    invokestatic  Test10User.doInvoke(LTest10Interface;)V
+    return
+.end method
+
+.method public static doInvoke(LTest10Interface;)V
+    .limit stack 3
+    .limit locals 3
+    aload_0
+    invokeinterface Test10Interface.clone()Ljava.lang.Object; 1
+    pop
+    return
+.end method
+
diff --git a/test/162-method-resolution/multidex.jpp b/test/162-method-resolution/multidex.jpp
index 22e3aeeadc..5722f7f1d8 100644
--- a/test/162-method-resolution/multidex.jpp
+++ b/test/162-method-resolution/multidex.jpp
@@ -112,6 +112,16 @@ Test9User2:
   @@com.android.jack.annotations.ForceInMainDex
   class Test9User2
 
+Test10Base:
+  @@com.android.jack.annotations.ForceInMainDex
+  class Test10Base
+Test10Interface:
+  @@com.android.jack.annotations.ForceInMainDex
+  class Test10Interface
+Test10User:
+  @@com.android.jack.annotations.ForceInMainDex
+  class Test10User
+
 Main:
   @@com.android.jack.annotations.ForceInMainDex
   class Main
diff --git a/test/162-method-resolution/src/Main.java b/test/162-method-resolution/src/Main.java
index fa95aa755c..864c87850b 100644
--- a/test/162-method-resolution/src/Main.java
+++ b/test/162-method-resolution/src/Main.java
@@ -36,6 +36,7 @@ public class Main {
             test7();
             test8();
             test9();
+            test10();
 
             // TODO: How to test that interface method resolution returns the unique
             // maximally-specific non-abstract superinterface method if there is one?
@@ -376,6 +377,31 @@ public class Main {
         invokeUserTest("Test9User2");
     }
 
+    /*
+     * Test10
+     * -----
+     * Tested function:
+     *     public class Test10Base implements Test10Interface { }
+     *     public interface Test10Interface { }
+     * Tested invokes:
+     *     invoke-interface Test10Interface.clone()Ljava/lang/Object; from Test10Caller in first dex
+     *         TODO b/64274113 This should throw a NSME (JLS 13.4.12) but actually throws an ICCE.
+     *         expected: Throws NoSuchMethodError (JLS 13.4.12)
+     *         actual: Throws IncompatibleClassChangeError
+     *
+     * This test is simulating compiling Test10Interface with "public Object clone()" method, along
+     * with every other class. Then we delete "clone" from Test10Interface only, which under JLS
+     * 13.4.12 is expected to be binary incompatible and throw a NoSuchMethodError.
+     *
+     * Files:
+     *   jasmin/Test10Base.j          - implements Test10Interface
+     *   jasmin/Test10Interface.java  - defines empty interface
+     *   jasmin/Test10User.j          - invokeinterface Test10Interface.clone()Ljava/lang/Object;
+     */
+    private static void test10() throws Exception {
+        invokeUserTest("Test10User");
+    }
+
     private static void invokeUserTest(String userName) throws Exception {
         System.out.println("Calling " + userName + ".test():");
         try {
diff --git a/test/162-method-resolution/src/Test10Interface.java b/test/162-method-resolution/src/Test10Interface.java
new file mode 100644
index 0000000000..3c75ea59be
--- /dev/null
+++ b/test/162-method-resolution/src/Test10Interface.java
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2017 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.
+ */
+
+public interface Test10Interface { }
+