ART: Handle unresolved catch handler types differently in the verifier
The move-exception should not trigger a synthetic throw, as that
may be caught by another catch handler, while the code is unreachable.
Use a specific flag and failure instead.
Bug: 134061982
Test: art/test/testrunner/testrunner.py -b --host -t 800
Change-Id: Ie6859e3a1910171b34882889ebece6cadc9dd508
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 0cdf010..5beea4f 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -112,8 +112,9 @@
namespace {
enum class CheckAccess {
- kYes,
kNo,
+ kOnResolvedClass,
+ kYes,
};
enum class FieldAccessType {
@@ -592,6 +593,8 @@
}
void Dump(VariableIndentationOutputStream* vios) REQUIRES_SHARED(Locks::mutator_lock_);
+ bool HandleMoveException(const Instruction* inst) REQUIRES_SHARED(Locks::mutator_lock_);
+
ArtMethod* method_being_verified_; // Its ArtMethod representation if known.
const uint32_t method_access_flags_; // Method's access flags.
const RegType* return_type_; // Lazily computed return type of the method.
@@ -2065,6 +2068,7 @@
saved_line_->FillWithGarbage();
}
DCHECK(!have_pending_runtime_throw_failure_); // Per-instruction flag, should not be set here.
+ bool exc_handler_unreachable = false;
// We need to ensure the work line is consistent while performing validation. When we spot a
@@ -2134,21 +2138,12 @@
work_line_->CopyResultRegister1(this, inst->VRegA_11x(), true);
break;
- case Instruction::MOVE_EXCEPTION: {
- // We do not allow MOVE_EXCEPTION as the first instruction in a method. This is a simple case
- // where one entrypoint to the catch block is not actually an exception path.
- if (work_insn_idx_ == 0) {
- Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "move-exception at pc 0x0";
- break;
+ case Instruction::MOVE_EXCEPTION:
+ if (!HandleMoveException(inst)) {
+ exc_handler_unreachable = true;
}
- /*
- * This statement can only appear as the first instruction in an exception handler. We verify
- * that as part of extracting the exception type from the catch block list.
- */
- const RegType& res_type = GetCaughtExceptionType();
- work_line_->SetRegisterType<LockOp::kClear>(this, inst->VRegA_11x(), res_type);
break;
- }
+
case Instruction::RETURN_VOID:
if (!IsInstanceConstructor() || work_line_->CheckConstructorReturn(this)) {
if (!GetMethodReturnType().IsConflict()) {
@@ -3490,6 +3485,7 @@
info_messages_ << "Rejecting opcode " << inst->DumpString(dex_file_);
return false;
} else if (have_pending_runtime_throw_failure_) {
+ LogVerifyInfo() << "Elevating opcode flags from " << opcode_flags << " to Throw";
/* checking interpreter will throw, mark following code as unreachable */
opcode_flags = Instruction::kThrow;
// Note: the flag must be reset as it is only global to decouple Fail and is semantically per
@@ -3649,7 +3645,7 @@
* because it changes work_line_ when performing peephole optimization
* and this change should not be used in those cases.
*/
- if ((opcode_flags & Instruction::kContinue) != 0) {
+ if ((opcode_flags & Instruction::kContinue) != 0 && !exc_handler_unreachable) {
DCHECK_EQ(&code_item_accessor_.InstructionAt(work_insn_idx_), inst);
uint32_t next_insn_idx = work_insn_idx_ + inst->SizeInCodeUnits();
if (next_insn_idx >= code_item_accessor_.InsnsSizeInCodeUnits()) {
@@ -3762,9 +3758,10 @@
// the access-checks interpreter. If result is primitive, skip the access check.
//
// Note: we do this for unresolved classes to trigger re-verification at runtime.
- if (C == CheckAccess::kYes &&
+ if (C != CheckAccess::kNo &&
result->IsNonZeroReferenceTypes() &&
- (IsSdkVersionSetAndAtLeast(api_level_, SdkVersion::kP) || !result->IsUnresolvedTypes())) {
+ ((C == CheckAccess::kYes && IsSdkVersionSetAndAtLeast(api_level_, SdkVersion::kP))
+ || !result->IsUnresolvedTypes())) {
const RegType& referrer = GetDeclaringClass();
if ((IsSdkVersionSetAndAtLeast(api_level_, SdkVersion::kP) || !referrer.IsUnresolvedTypes()) &&
!referrer.CanAccess(*result)) {
@@ -3776,55 +3773,88 @@
}
template <bool kVerifierDebug>
-const RegType& MethodVerifier<kVerifierDebug>::GetCaughtExceptionType() {
- const RegType* common_super = nullptr;
- if (code_item_accessor_.TriesSize() != 0) {
- const uint8_t* handlers_ptr = code_item_accessor_.GetCatchHandlerData();
- uint32_t handlers_size = DecodeUnsignedLeb128(&handlers_ptr);
- for (uint32_t i = 0; i < handlers_size; i++) {
- CatchHandlerIterator iterator(handlers_ptr);
- for (; iterator.HasNext(); iterator.Next()) {
- if (iterator.GetHandlerAddress() == (uint32_t) work_insn_idx_) {
- if (!iterator.GetHandlerTypeIndex().IsValid()) {
- common_super = ®_types_.JavaLangThrowable(false);
- } else {
- const RegType& exception =
- ResolveClass<CheckAccess::kYes>(iterator.GetHandlerTypeIndex());
- if (!reg_types_.JavaLangThrowable(false).IsAssignableFrom(exception, this)) {
- DCHECK(!exception.IsUninitializedTypes()); // Comes from dex, shouldn't be uninit.
- if (exception.IsUnresolvedTypes()) {
- // We don't know enough about the type. Fail here and let runtime handle it.
- Fail(VERIFY_ERROR_NO_CLASS) << "unresolved exception class " << exception;
- return exception;
- } else {
- Fail(VERIFY_ERROR_BAD_CLASS_SOFT) << "unexpected non-exception class " << exception;
- return reg_types_.Conflict();
- }
- } else if (common_super == nullptr) {
- common_super = &exception;
- } else if (common_super->Equals(exception)) {
- // odd case, but nothing to do
+bool MethodVerifier<kVerifierDebug>::HandleMoveException(const Instruction* inst) {
+ // We do not allow MOVE_EXCEPTION as the first instruction in a method. This is a simple case
+ // where one entrypoint to the catch block is not actually an exception path.
+ if (work_insn_idx_ == 0) {
+ Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "move-exception at pc 0x0";
+ return true;
+ }
+ /*
+ * This statement can only appear as the first instruction in an exception handler. We verify
+ * that as part of extracting the exception type from the catch block list.
+ */
+ auto caught_exc_type_fn = [&]() REQUIRES_SHARED(Locks::mutator_lock_) ->
+ std::pair<bool, const RegType*> {
+ const RegType* common_super = nullptr;
+ if (code_item_accessor_.TriesSize() != 0) {
+ const uint8_t* handlers_ptr = code_item_accessor_.GetCatchHandlerData();
+ uint32_t handlers_size = DecodeUnsignedLeb128(&handlers_ptr);
+ const RegType* unresolved = nullptr;
+ for (uint32_t i = 0; i < handlers_size; i++) {
+ CatchHandlerIterator iterator(handlers_ptr);
+ for (; iterator.HasNext(); iterator.Next()) {
+ if (iterator.GetHandlerAddress() == (uint32_t) work_insn_idx_) {
+ if (!iterator.GetHandlerTypeIndex().IsValid()) {
+ common_super = ®_types_.JavaLangThrowable(false);
} else {
- common_super = &common_super->Merge(exception, ®_types_, this);
- if (FailOrAbort(reg_types_.JavaLangThrowable(false).IsAssignableFrom(
- *common_super, this),
- "java.lang.Throwable is not assignable-from common_super at ",
- work_insn_idx_)) {
- break;
+ // Do access checks only on resolved exception classes.
+ const RegType& exception =
+ ResolveClass<CheckAccess::kOnResolvedClass>(iterator.GetHandlerTypeIndex());
+ if (!reg_types_.JavaLangThrowable(false).IsAssignableFrom(exception, this)) {
+ DCHECK(!exception.IsUninitializedTypes()); // Comes from dex, shouldn't be uninit.
+ if (exception.IsUnresolvedTypes()) {
+ if (unresolved == nullptr) {
+ unresolved = &exception;
+ } else {
+ unresolved = &unresolved->SafeMerge(exception, ®_types_, this);
+ }
+ } else {
+ Fail(VERIFY_ERROR_BAD_CLASS_SOFT) << "unexpected non-exception class "
+ << exception;
+ return std::make_pair(true, ®_types_.Conflict());
+ }
+ } else if (common_super == nullptr) {
+ common_super = &exception;
+ } else if (common_super->Equals(exception)) {
+ // odd case, but nothing to do
+ } else {
+ common_super = &common_super->Merge(exception, ®_types_, this);
+ if (FailOrAbort(reg_types_.JavaLangThrowable(false).IsAssignableFrom(
+ *common_super, this),
+ "java.lang.Throwable is not assignable-from common_super at ",
+ work_insn_idx_)) {
+ break;
+ }
}
}
}
}
+ handlers_ptr = iterator.EndDataPointer();
}
- handlers_ptr = iterator.EndDataPointer();
+ if (unresolved != nullptr) {
+ if (!Runtime::Current()->IsAotCompiler() && common_super == nullptr) {
+ // This is an unreachable handler.
+ return std::make_pair(false, unresolved);
+ }
+ // Soft-fail, but do not handle this with a synthetic throw.
+ Fail(VERIFY_ERROR_UNRESOLVED_CATCH) << "Unresolved catch handler";
+ if (common_super != nullptr) {
+ unresolved = &unresolved->Merge(*common_super, ®_types_, this);
+ }
+ return std::make_pair(true, unresolved);
+ }
}
- }
- if (common_super == nullptr) {
- /* no catch blocks, or no catches with classes we can find */
- Fail(VERIFY_ERROR_BAD_CLASS_SOFT) << "unable to find exception handler";
- return reg_types_.Conflict();
- }
- return *common_super;
+ if (common_super == nullptr) {
+ /* no catch blocks, or no catches with classes we can find */
+ Fail(VERIFY_ERROR_BAD_CLASS_SOFT) << "unable to find exception handler";
+ return std::make_pair(true, ®_types_.Conflict());
+ }
+ return std::make_pair(true, common_super);
+ };
+ auto result = caught_exc_type_fn();
+ work_line_->SetRegisterType<LockOp::kClear>(this, inst->VRegA_11x(), *result.second);
+ return result.first;
}
template <bool kVerifierDebug>
@@ -5547,6 +5577,10 @@
have_pending_hard_failure_ = true;
break;
}
+
+ case VERIFY_ERROR_UNRESOLVED_CATCH:
+ // Nothing to do, just remember the failure type.
+ break;
}
failures_.push_back(error);
std::string location(StringPrintf("%s: [0x%X] ", dex_file_->PrettyMethod(dex_method_idx_).c_str(),
diff --git a/runtime/verifier/verifier_enums.h b/runtime/verifier/verifier_enums.h
index bbdd45d..1b07da8 100644
--- a/runtime/verifier/verifier_enums.h
+++ b/runtime/verifier/verifier_enums.h
@@ -93,6 +93,9 @@
// (sets a soft fail at compile time).
VERIFY_ERROR_LOCKING = 2048, // Could not guarantee balanced locking. This should be
// punted to the interpreter with access checks.
+ VERIFY_ERROR_UNRESOLVED_CATCH = 4096, // Error code necessary to have a synthetic soft fail
+ // that is not an exception, to let the compiler know
+ // that there is (unreachable) unverified code.
};
std::ostream& operator<<(std::ostream& os, const VerifyError& rhs);
diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt
index 63859fe..88dc054 100644
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -75,4 +75,6 @@
ConstClassAliasing
b/121191566
b/122501785
+b/134061982
+b/134061982 (2)
Done!
diff --git a/test/800-smali/smali/b_134061982.smali b/test/800-smali/smali/b_134061982.smali
new file mode 100644
index 0000000..c62fa8b
--- /dev/null
+++ b/test/800-smali/smali/b_134061982.smali
@@ -0,0 +1,60 @@
+.class public LB134061982;
+.super Ljava/lang/Object;
+
+
+.method public constructor <init>()V
+.registers 1
+ invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+ return-void
+.end method
+
+.method public static run(I)V
+.registers 4
+
+# Registers:
+# * v0 = 0/null
+# * v1 = "outer" catch value to operate on
+# * v2 = exception value for inner catch
+# * v3 = p0 = input for two legs.
+
+ const v0, 0
+
+ # Start with r1 == null
+ const v1, 0
+
+ if-eqz p0, :direct_leg
+ goto :indirect_leg
+
+:direct_leg
+ throw v0
+
+:indirect_leg
+ # Make r1 not-reference.
+ const v1, 1
+ throw v0
+
+:end
+ return-void
+
+:catch_inner
+ move-exception v2
+ # r2 should not be primitive, so this should hard-fail if reached.
+ add-int/lit8 v2, v2, 0x1
+ goto :end
+
+:catch_outer
+ # Just some random call.
+ invoke-virtual {v1}, Ljava/io/PrintStream;->println()V
+ goto :end
+
+# Direct leg is directly covered by catch_outer.
+.catchall {:direct_leg .. :indirect_leg} :catch_outer
+
+# Indirect leg is directly covered by catch_inner.
+# * Covered by unresolved exception class -> unreachable.
+.catch Ldoes/not/ResolveException; {:indirect_leg .. :end} :catch_inner
+
+# catch_inner is covered by catch_outer.
+.catchall {:catch_inner .. :catch_outer} :catch_outer
+
+.end method
diff --git a/test/800-smali/smali/b_134061983_2.smali b/test/800-smali/smali/b_134061983_2.smali
new file mode 100644
index 0000000..a7ad684
--- /dev/null
+++ b/test/800-smali/smali/b_134061983_2.smali
@@ -0,0 +1,61 @@
+.class public LB134061982_2;
+.super Ljava/lang/Object;
+
+
+.method public constructor <init>()V
+.registers 1
+ invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+ return-void
+.end method
+
+.method public static run(I)V
+.registers 4
+
+# Registers:
+# * v0 = 0/null
+# * v1 = "outer" catch value to operate on
+# * v2 = exception value for inner catch
+# * v3 = p0 = input for two legs.
+
+ const v0, 0
+
+ # Start with r1 == null
+ const v1, 0
+
+ if-eqz p0, :direct_leg
+ goto :indirect_leg
+
+:direct_leg
+ throw v0
+
+:indirect_leg
+ # Make r1 not-reference.
+ const v1, 1
+ throw v0
+
+:end
+ return-void
+
+:catch_inner
+ move-exception v2
+ # r2 should not be primitive, so this should hard-fail if reached.
+ add-int/lit8 v2, v2, 0x1
+ goto :end
+
+:catch_outer
+ # Just some random call.
+ invoke-virtual {v1}, Ljava/io/PrintStream;->println()V
+ goto :end
+
+# Direct leg is directly covered by catch_outer.
+.catchall {:direct_leg .. :indirect_leg} :catch_outer
+
+# Indirect leg is directly covered by catch_inner.
+# * Covered by unresolved and resolved exception classes -> live.
+.catch Ldoes/not/ResolveException; {:indirect_leg .. :end} :catch_inner
+.catch Ljava/lang/ArithmeticException; {:indirect_leg .. :end} :catch_inner
+
+# catch_inner is covered by catch_outer.
+.catchall {:catch_inner .. :catch_outer} :catch_outer
+
+.end method
diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java
index d10fdf1..ce7426f 100644
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -197,6 +197,10 @@
true, false));
testCases.add(new TestCase("b/122501785", "B122501785", "run", null, new VerifyError(),
0));
+ testCases.add(new TestCase("b/134061982", "B134061982", "run", new Object[] { 0 },
+ new NullPointerException(), 0));
+ testCases.add(new TestCase("b/134061982 (2)", "B134061982_2", "run", new Object[] { 0 },
+ new VerifyError(), 0));
}
public void runTests() {