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

7 files changed, 96 insertions, 96 deletions

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 @@ ALWAYS_INLINE static inline bool FailOrAbort(MethodVerifier* verifier, bool cond
 }
 
 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 @@ bool MethodVerifier::SetTypesFromSignature() {
     // 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 @@ bool MethodVerifier::CodeFlowVerifyInstruction(uint32_t* start_guess) {
   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 @@ bool MethodVerifier::CodeFlowVerifyInstruction(uint32_t* start_guess) {
       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 @@ bool MethodVerifier::CodeFlowVerifyInstruction(uint32_t* start_guess) {
       }
       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 @@ bool MethodVerifier::CodeFlowVerifyInstruction(uint32_t* start_guess) {
       }
       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 @@ bool MethodVerifier::CodeFlowVerifyInstruction(uint32_t* start_guess) {
      */
   }  // 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 @@ bool MethodVerifier::UpdateRegisters(uint32_t next_insn, RegisterLine* merge_lin
       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 @@ class MethodVerifier {
     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 @@ void RegisterLine::MarkRefsAsInitialized(MethodVerifier* verifier, const RegType
       }
     }
   }
+  // Is this initializing "this"?
+  if (uninit_type.IsUninitializedThisReference() ||
+      uninit_type.IsUnresolvedAndUninitializedThisReference()) {
+    this_initialized_ = true;
+  }
   DCHECK_GT(changed, 0u);
 }
 
@@ -432,6 +401,11 @@ bool RegisterLine::MergeRegisters(MethodVerifier* verifier, const RegisterLine*
       }
     }
   }
+  // 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 @@ class RegisterLine {
     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 @@ class RegisterLine {
   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 @@ class RegisterLine {
    */
   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 @@ class RegisterLine {
   }
 
   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 @@ class RegisterLine {
   // 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
 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 @@ public class Main {
         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() {