Do not hide instance field hard failure with soft failure
Rationale:
Yet another verifier inaccuracy found with fuzz testing.
Instance field verification should proceed testing instance
field access after soft failures in cases where hard failures
could still follow. Failure to do so resulted in a compiler
crash (now made bit friendly with DCHECK as well).
With crash-before/pass-after test.
BUG=29126870
Change-Id: I8674d6171158eaa2aeb0492b35dfafea76416cac
diff --git a/compiler/optimizing/instruction_builder.cc b/compiler/optimizing/instruction_builder.cc
index 5e691c7..135038b 100644
--- a/compiler/optimizing/instruction_builder.cc
+++ b/compiler/optimizing/instruction_builder.cc
@@ -381,10 +381,11 @@
// If the operation requests a specific type, we make sure its input is of that type.
if (type != value->GetType()) {
if (Primitive::IsFloatingPointType(type)) {
- return ssa_builder_->GetFloatOrDoubleEquivalent(value, type);
+ value = ssa_builder_->GetFloatOrDoubleEquivalent(value, type);
} else if (type == Primitive::kPrimNot) {
- return ssa_builder_->GetReferenceTypeEquivalent(value);
+ value = ssa_builder_->GetReferenceTypeEquivalent(value);
}
+ DCHECK(value != nullptr);
}
return value;
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 8ad79fb..f2ae85a 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -4528,7 +4528,7 @@
ArtField* MethodVerifier::GetInstanceField(const RegType& obj_type, int field_idx) {
const DexFile::FieldId& field_id = dex_file_->GetFieldId(field_idx);
- // Check access to class
+ // Check access to class.
const RegType& klass_type = ResolveClassAndCheckAccess(field_id.class_idx_);
if (klass_type.IsConflict()) {
AppendToLastFailMessage(StringPrintf(" in attempt to access instance field %d (%s) in %s",
@@ -4549,20 +4549,11 @@
DCHECK(self_->IsExceptionPending());
self_->ClearException();
return nullptr;
- } else if (!GetDeclaringClass().CanAccessMember(field->GetDeclaringClass(),
- field->GetAccessFlags())) {
- Fail(VERIFY_ERROR_ACCESS_FIELD) << "cannot access instance field " << PrettyField(field)
- << " from " << GetDeclaringClass();
- return nullptr;
- } else if (field->IsStatic()) {
- Fail(VERIFY_ERROR_CLASS_CHANGE) << "expected field " << PrettyField(field)
- << " to not be static";
- return nullptr;
} else if (obj_type.IsZero()) {
- // Cannot infer and check type, however, access will cause null pointer exception
- return field;
+ // Cannot infer and check type, however, access will cause null pointer exception.
+ // Fall through into a few last soft failure checks below.
} else if (!obj_type.IsReferenceTypes()) {
- // Trying to read a field from something that isn't a reference
+ // Trying to read a field from something that isn't a reference.
Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "instance field access on object that has "
<< "non-reference type " << obj_type;
return nullptr;
@@ -4584,7 +4575,6 @@
<< " of " << PrettyMethod(dex_method_idx_, *dex_file_);
return nullptr;
}
- return field;
} else if (!field_klass.IsAssignableFrom(obj_type)) {
// Trying to access C1.field1 using reference of type C2, which is neither C1 or a sub-class
// of C1. For resolution to occur the declared class of the field must be compatible with
@@ -4602,10 +4592,22 @@
Fail(type) << "cannot access instance field " << PrettyField(field)
<< " from object of type " << obj_type;
return nullptr;
- } else {
- return field;
}
}
+
+ // Few last soft failure checks.
+ if (!GetDeclaringClass().CanAccessMember(field->GetDeclaringClass(),
+ field->GetAccessFlags())) {
+ Fail(VERIFY_ERROR_ACCESS_FIELD) << "cannot access instance field " << PrettyField(field)
+ << " from " << GetDeclaringClass();
+ return nullptr;
+ } else if (field->IsStatic()) {
+ Fail(VERIFY_ERROR_CLASS_CHANGE) << "expected field " << PrettyField(field)
+ << " to not be static";
+ return nullptr;
+ }
+
+ return field;
}
template <MethodVerifier::FieldAccessType kAccType>
@@ -4928,6 +4930,7 @@
// Initialize them as conflicts so they don't add to GC and deoptimization information.
const Instruction* ret_inst = Instruction::At(code_item_->insns_ + next_insn);
AdjustReturnLine(this, ret_inst, target_line);
+ // Directly bail if a hard failure was found.
if (have_pending_hard_failure_) {
return false;
}
diff --git a/test/600-verifier-fails/expected.txt b/test/600-verifier-fails/expected.txt
index 010f1b7..8399969 100644
--- a/test/600-verifier-fails/expected.txt
+++ b/test/600-verifier-fails/expected.txt
@@ -1,3 +1,4 @@
passed A
passed B
passed C
+passed D
diff --git a/test/600-verifier-fails/info.txt b/test/600-verifier-fails/info.txt
index 677b477..f77de05 100644
--- a/test/600-verifier-fails/info.txt
+++ b/test/600-verifier-fails/info.txt
@@ -1,14 +1,18 @@
The situations in these tests were discovered by running the mutating
dexfuzz on the DEX files of fuzzingly random generated Java test.
-(1) b/28908555:
- soft verification fail (on the final field modification) should
- not hide the hard verification fail (on the type mismatch) to
+(A) b/28908555:
+ soft verification failure (on the final field modification) should
+ not hide the hard verification failure (on the type mismatch) to
avoid compiler crash later on
-(2) b/29070461:
- hard failure (not calling super in constructor) should bail
- immediately and not allow soft fails to pile up behind it to
- avoid fatal message later on
-(3) b/29068831:
+(B) b/29070461:
+ hard verification failure (not calling super in constructor) should
+ bail immediately and not allow soft verification failures to pile up
+ behind it to avoid fatal message later on
+(C) b/29068831:
access validation should occur prior to null reference check
+(D) b/29126870:
+ soft verification failure (cannot access) should not hide the hard
+ verification failure (non-reference type) to avoid a compiler crash
+ later on
diff --git a/test/600-verifier-fails/smali/iget.smali b/test/600-verifier-fails/smali/iget.smali
new file mode 100644
index 0000000..5c045e6
--- /dev/null
+++ b/test/600-verifier-fails/smali/iget.smali
@@ -0,0 +1,25 @@
+#
+# Copyright (C) 2016 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 LD;
+.super Ljava/lang/Object;
+
+.method public constructor <init>()V
+ .registers 2
+ invoke-direct {v1}, Ljava/lang/Object;-><init>()V
+ const v0, 2
+ iget v1, v0, LMain;->privateField:I
+ return-void
+.end method
diff --git a/test/600-verifier-fails/src/Main.java b/test/600-verifier-fails/src/Main.java
index 0a8c5a1..a6a07fd 100644
--- a/test/600-verifier-fails/src/Main.java
+++ b/test/600-verifier-fails/src/Main.java
@@ -20,6 +20,8 @@
private static String staticPrivateField = null;
+ private int privateField = 0;
+
private static void test(String name) throws Exception {
try {
Class<?> a = Class.forName(name);
@@ -33,5 +35,6 @@
test("A");
test("B");
test("C");
+ test("D");
}
}