ART: Change UninitializedThis tracking in the verifier
Only relying on register types is error-prone. For example, we may
inadvertently reject correct code when the constructor terminates
abnormally.
Bug: 20843113
(cherry picked from commit f10b6e109bfb595b6752d1b59db680694ac1684d)
(cherry picked from commit af31802e5b74f5b9b8d3aadbaaf48cfde14ff7d1)
Change-Id: I8826cd167780df25a6166740f183d216483fa550
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index e3999c1..015e908 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -107,7 +107,7 @@
}
static void SafelyMarkAllRegistersAsConflicts(MethodVerifier* verifier, RegisterLine* reg_line) {
- if (verifier->IsConstructor()) {
+ if (verifier->IsInstanceConstructor()) {
// Before we mark all regs as conflicts, check that we don't have an uninitialized this.
reg_line->CheckConstructorReturn(verifier);
}
@@ -1329,9 +1329,15 @@
// argument as uninitialized. This restricts field access until the superclass constructor is
// called.
const RegType& declaring_class = GetDeclaringClass();
- if (IsConstructor() && !declaring_class.IsJavaLangObject()) {
- reg_line->SetRegisterType(this, arg_start + cur_arg,
- reg_types_.UninitializedThisArgument(declaring_class));
+ if (IsConstructor()) {
+ if (declaring_class.IsJavaLangObject()) {
+ // "this" is implicitly initialized.
+ reg_line->SetThisInitialized();
+ reg_line->SetRegisterType(this, arg_start + cur_arg, declaring_class);
+ } else {
+ reg_line->SetRegisterType(this, arg_start + cur_arg,
+ reg_types_.UninitializedThisArgument(declaring_class));
+ }
} else {
reg_line->SetRegisterType(this, arg_start + cur_arg, declaring_class);
}
@@ -1654,16 +1660,6 @@
std::unique_ptr<RegisterLine> branch_line;
std::unique_ptr<RegisterLine> fallthrough_line;
- /*
- * If we are in a constructor, and we currently have an UninitializedThis type
- * in a register somewhere, we need to make sure it isn't overwritten.
- */
- bool track_uninitialized_this = false;
- size_t uninitialized_this_loc = 0;
- if (IsConstructor()) {
- track_uninitialized_this = work_line_->GetUninitializedThisLoc(this, &uninitialized_this_loc);
- }
-
switch (inst->Opcode()) {
case Instruction::NOP:
/*
@@ -1741,14 +1737,14 @@
break;
}
case Instruction::RETURN_VOID:
- if (!IsConstructor() || work_line_->CheckConstructorReturn(this)) {
+ if (!IsInstanceConstructor() || work_line_->CheckConstructorReturn(this)) {
if (!GetMethodReturnType().IsConflict()) {
Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "return-void not expected";
}
}
break;
case Instruction::RETURN:
- if (!IsConstructor() || work_line_->CheckConstructorReturn(this)) {
+ if (!IsInstanceConstructor() || work_line_->CheckConstructorReturn(this)) {
/* check the method signature */
const RegType& return_type = GetMethodReturnType();
if (!return_type.IsCategory1Types()) {
@@ -1773,7 +1769,7 @@
}
break;
case Instruction::RETURN_WIDE:
- if (!IsConstructor() || work_line_->CheckConstructorReturn(this)) {
+ if (!IsInstanceConstructor() || work_line_->CheckConstructorReturn(this)) {
/* check the method signature */
const RegType& return_type = GetMethodReturnType();
if (!return_type.IsCategory2Types()) {
@@ -1789,7 +1785,7 @@
}
break;
case Instruction::RETURN_OBJECT:
- if (!IsConstructor() || work_line_->CheckConstructorReturn(this)) {
+ if (!IsInstanceConstructor() || work_line_->CheckConstructorReturn(this)) {
const RegType& return_type = GetMethodReturnType();
if (!return_type.IsReferenceTypes()) {
Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "return-object not expected";
@@ -2912,20 +2908,6 @@
*/
} // end - switch (dec_insn.opcode)
- /*
- * If we are in a constructor, and we had an UninitializedThis type
- * in a register somewhere, we need to make sure it wasn't overwritten.
- */
- if (track_uninitialized_this) {
- bool was_invoke_direct = (inst->Opcode() == Instruction::INVOKE_DIRECT ||
- inst->Opcode() == Instruction::INVOKE_DIRECT_RANGE);
- if (work_line_->WasUninitializedThisOverwritten(this, uninitialized_this_loc,
- was_invoke_direct)) {
- Fail(VERIFY_ERROR_BAD_CLASS_HARD)
- << "Constructor failed to initialize this object";
- }
- }
-
if (have_pending_hard_failure_) {
if (Runtime::Current()->IsAotCompiler()) {
/* When AOT compiling, check that the last failure is a hard failure */
@@ -4268,6 +4250,10 @@
const Instruction* ret_inst = Instruction::At(code_item_->insns_ + next_insn);
Instruction::Code opcode = ret_inst->Opcode();
if (opcode == Instruction::RETURN_VOID || opcode == Instruction::RETURN_VOID_NO_BARRIER) {
+ // Explicitly copy the this-initialized flag from the merge-line, as we didn't copy its
+ // state. Must be done before SafelyMarkAllRegistersAsConflicts as that will do the
+ // super-constructor-call checking.
+ target_line->CopyThisInitialized(*merge_line);
SafelyMarkAllRegistersAsConflicts(this, target_line);
} else {
target_line->CopyFromLine(merge_line);
diff --git a/runtime/verifier/method_verifier.h b/runtime/verifier/method_verifier.h
index 62800fb..7b91461 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -269,6 +269,10 @@
return (method_access_flags_ & kAccStatic) != 0;
}
+ bool IsInstanceConstructor() const {
+ return IsConstructor() && !IsStatic();
+ }
+
SafeMap<uint32_t, std::set<uint32_t>>& GetStringInitPcRegMap() {
return string_init_pc_reg_map_;
}
diff --git a/runtime/verifier/register_line.cc b/runtime/verifier/register_line.cc
index 2838681..f286a45 100644
--- a/runtime/verifier/register_line.cc
+++ b/runtime/verifier/register_line.cc
@@ -18,66 +18,30 @@
#include "base/stringprintf.h"
#include "dex_instruction-inl.h"
-#include "method_verifier.h"
+#include "method_verifier-inl.h"
#include "register_line-inl.h"
#include "reg_type-inl.h"
namespace art {
namespace verifier {
-bool RegisterLine::WasUninitializedThisOverwritten(MethodVerifier* verifier,
- size_t this_loc,
- bool was_invoke_direct) const {
- DCHECK(verifier->IsConstructor());
-
- // Is the UnintializedThis type still there?
- if (GetRegisterType(verifier, this_loc).IsUninitializedThisReference() ||
- GetRegisterType(verifier, this_loc).IsUnresolvedAndUninitializedThisReference()) {
- return false;
- }
-
- // If there is an initialized reference here now, did we just perform an invoke-direct? Note that
- // this is the correct approach for dex bytecode: results of invoke-direct are stored in the
- // result register. Overwriting "this_loc" can only be done by a constructor call.
- if (GetRegisterType(verifier, this_loc).IsReferenceTypes() && was_invoke_direct) {
- return false;
- // Otherwise we could have just copied a different initialized reference to this location.
- }
-
- // The UnintializedThis in the register is gone, so check to see if it's somewhere else now.
- for (size_t i = 0; i < num_regs_; i++) {
- if (GetRegisterType(verifier, i).IsUninitializedThisReference() ||
- GetRegisterType(verifier, i).IsUnresolvedAndUninitializedThisReference()) {
- // We found it somewhere else...
- return false;
- }
- }
-
- // The UninitializedThis is gone from the original register, and now we can't find it.
- return true;
-}
-
-bool RegisterLine::GetUninitializedThisLoc(MethodVerifier* verifier, size_t* vreg) const {
- for (size_t i = 0; i < num_regs_; i++) {
- if (GetRegisterType(verifier, i).IsUninitializedThisReference() ||
- GetRegisterType(verifier, i).IsUnresolvedAndUninitializedThisReference()) {
- *vreg = i;
- return true;
- }
- }
- return false;
-}
-
bool RegisterLine::CheckConstructorReturn(MethodVerifier* verifier) const {
- for (size_t i = 0; i < num_regs_; i++) {
- if (GetRegisterType(verifier, i).IsUninitializedThisReference() ||
- GetRegisterType(verifier, i).IsUnresolvedAndUninitializedThisReference()) {
- verifier->Fail(VERIFY_ERROR_BAD_CLASS_SOFT)
- << "Constructor returning without calling superclass constructor";
- return false;
+ if (kIsDebugBuild && this_initialized_) {
+ // Ensure that there is no UninitializedThisReference type anymore if this_initialized_ is true.
+ for (size_t i = 0; i < num_regs_; i++) {
+ const RegType& type = GetRegisterType(verifier, i);
+ CHECK(!type.IsUninitializedThisReference() &&
+ !type.IsUnresolvedAndUninitializedThisReference())
+ << i << ": " << type.IsUninitializedThisReference() << " in "
+ << PrettyMethod(verifier->GetMethodReference().dex_method_index,
+ *verifier->GetMethodReference().dex_file);
}
}
- return true;
+ if (!this_initialized_) {
+ verifier->Fail(VERIFY_ERROR_BAD_CLASS_HARD)
+ << "Constructor returning without calling superclass constructor";
+ }
+ return this_initialized_;
}
const RegType& RegisterLine::GetInvocationThis(MethodVerifier* verifier, const Instruction* inst,
@@ -148,6 +112,11 @@
}
}
}
+ // Is this initializing "this"?
+ if (uninit_type.IsUninitializedThisReference() ||
+ uninit_type.IsUnresolvedAndUninitializedThisReference()) {
+ this_initialized_ = true;
+ }
DCHECK_GT(changed, 0u);
}
@@ -432,6 +401,11 @@
}
}
}
+ // Check whether "this" was initialized in both paths.
+ if (this_initialized_ && !incoming_line->this_initialized_) {
+ this_initialized_ = false;
+ changed = true;
+ }
return changed;
}
diff --git a/runtime/verifier/register_line.h b/runtime/verifier/register_line.h
index 0de0d9c..366bca2 100644
--- a/runtime/verifier/register_line.h
+++ b/runtime/verifier/register_line.h
@@ -114,6 +114,7 @@
memcpy(&line_, &src->line_, num_regs_ * sizeof(uint16_t));
monitors_ = src->monitors_;
reg_to_lock_depths_ = src->reg_to_lock_depths_;
+ this_initialized_ = src->this_initialized_;
}
std::string Dump(MethodVerifier* verifier) const SHARED_LOCKS_REQUIRED(Locks::mutator_lock_);
@@ -149,6 +150,14 @@
void MarkAllRegistersAsConflictsExcept(MethodVerifier* verifier, uint32_t vsrc);
void MarkAllRegistersAsConflictsExceptWide(MethodVerifier* verifier, uint32_t vsrc);
+ void SetThisInitialized() {
+ this_initialized_ = true;
+ }
+
+ void CopyThisInitialized(const RegisterLine& src) {
+ this_initialized_ = src.this_initialized_;
+ }
+
/*
* Check constraints on constructor return. Specifically, make sure that the "this" argument got
* initialized.
@@ -158,18 +167,6 @@
*/
bool CheckConstructorReturn(MethodVerifier* verifier) const;
- /*
- * Check if an UninitializedThis at the specified location has been overwritten before
- * being correctly initialized.
- */
- bool WasUninitializedThisOverwritten(MethodVerifier* verifier, size_t this_loc,
- bool was_invoke_direct) const;
-
- /*
- * Get the first location of an UninitializedThis type, or return kInvalidVreg if there are none.
- */
- bool GetUninitializedThisLoc(MethodVerifier* verifier, size_t* vreg) const;
-
// Compare two register lines. Returns 0 if they match.
// Using this for a sort is unwise, since the value can change based on machine endianness.
int CompareLine(const RegisterLine* line2) const {
@@ -354,7 +351,7 @@
}
RegisterLine(size_t num_regs, MethodVerifier* verifier)
- : num_regs_(num_regs) {
+ : num_regs_(num_regs), this_initialized_(false) {
memset(&line_, 0, num_regs_ * sizeof(uint16_t));
SetResultTypeToUnknown(verifier);
}
@@ -372,6 +369,9 @@
// monitor-enter on v5 and then on v6, we expect the monitor-exit to be on v6 then on v5.
AllocationTrackingSafeMap<uint32_t, uint32_t, kAllocatorTagVerifier> reg_to_lock_depths_;
+ // Whether "this" initialization (a constructor supercall) has happened.
+ bool this_initialized_;
+
// An array of RegType Ids associated with each dex register.
uint16_t line_[0];
diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt
index 77668da..ebcaad1 100644
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -28,4 +28,5 @@
b/22331663 (pass)
b/22331663 (fail)
b/22881413
+b/20843113
Done!
diff --git a/test/800-smali/smali/b_20843113.smali b/test/800-smali/smali/b_20843113.smali
new file mode 100644
index 0000000..ab3dc41
--- /dev/null
+++ b/test/800-smali/smali/b_20843113.smali
@@ -0,0 +1,34 @@
+.class public LB20843113;
+.super Ljava/lang/Object;
+
+
+.method public constructor <init>(I)V
+.registers 2
+
+:Label1
+ # An instruction that may throw, so as to pass UninitializedThis to the handler
+ div-int v1, v1, v1
+
+ # Call the super-constructor
+ invoke-direct {v0}, Ljava/lang/Object;-><init>()V
+
+ # Return normally.
+ return-void
+
+:Label2
+
+
+:Handler
+ move-exception v0 # Overwrite the (last) "this" register. This should be
+ # allowed as we will terminate abnormally below.
+
+ throw v0 # Terminate abnormally
+
+.catchall {:Label1 .. :Label2} :Handler
+.end method
+
+# Just a dummy.
+.method public static run()V
+.registers 1
+ return-void
+.end method
diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java
index 7ee1e45..e487374 100644
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -102,6 +102,7 @@
testCases.add(new TestCase("b/22331663 (fail)", "B22331663Fail", "run",
new Object[] { false }, new VerifyError(), null));
testCases.add(new TestCase("b/22881413", "B22881413", "run", null, null, null));
+ testCases.add(new TestCase("b/20843113", "B20843113", "run", null, null, null));
}
public void runTests() {
