Fix UAF error caught by asan

We were 'delete'ing the AdbConnectionState while it still had a thread
running. To fix this we moved responsibility for deleting the
AdbConnectionState to the thread that makes use of it (if it was, in
fact, created).

Test: Build and boot device with adbconnection

Change-Id: I9f33a308d9b56a33c155b37dd86749d5a765809c

2 files changed, 26 insertions, 8 deletions

diff --git a/adbconnection/adbconnection.cc b/adbconnection/adbconnection.cc
index d8db923a28..634d6b56df 100644
--- a/adbconnection/adbconnection.cc
+++ b/adbconnection/adbconnection.cc
@@ -139,7 +139,8 @@ AdbConnectionState::AdbConnectionState(const std::string& agent_name)
     sent_agent_fds_(false),
     performed_handshake_(false),
     notified_ddm_active_(false),
-    next_ddm_id_(1) {
+    next_ddm_id_(1),
+    started_debugger_threads_(false) {
   // Setup the addr.
   control_addr_.controlAddrUn.sun_family = AF_UNIX;
   control_addr_len_ = sizeof(control_addr_.controlAddrUn.sun_family) + sizeof(kJdwpControlName) - 1;
@@ -174,6 +175,7 @@ struct CallbackData {
 
 static void* CallbackFunction(void* vdata) {
   std::unique_ptr<CallbackData> data(reinterpret_cast<CallbackData*>(vdata));
+  CHECK(data->this_ == gState);
   art::Thread* self = art::Thread::Attach(kAdbConnectionThreadName,
                                           true,
                                           data->thr_);
@@ -199,6 +201,10 @@ static void* CallbackFunction(void* vdata) {
   int detach_result = art::Runtime::Current()->GetJavaVM()->DetachCurrentThread();
   CHECK_EQ(detach_result, 0);
 
+  // Get rid of the connection
+  gState = nullptr;
+  delete data->this_;
+
   return nullptr;
 }
 
@@ -247,11 +253,13 @@ void AdbConnectionState::StartDebuggerThreads() {
   ScopedLocalRef<jobject> thr(soa.Env(), CreateAdbConnectionThread(soa.Self()));
   pthread_t pthread;
   std::unique_ptr<CallbackData> data(new CallbackData { this, soa.Env()->NewGlobalRef(thr.get()) });
+  started_debugger_threads_ = true;
   int pthread_create_result = pthread_create(&pthread,
                                              nullptr,
                                              &CallbackFunction,
                                              data.get());
   if (pthread_create_result != 0) {
+    started_debugger_threads_ = false;
     // If the create succeeded the other thread will call EndThreadBirth.
     art::Runtime* runtime = art::Runtime::Current();
     soa.Env()->DeleteGlobalRef(data->thr_);
@@ -545,6 +553,7 @@ bool AdbConnectionState::SetupAdbConnection() {
 }
 
 void AdbConnectionState::RunPollLoop(art::Thread* self) {
+  CHECK_NE(agent_name_, "");
   CHECK_EQ(self->GetState(), art::kNative);
   art::Locks::mutator_lock_->AssertNotHeld(self);
   self->SetState(art::kWaitingInMainDebuggerLoop);
@@ -869,24 +878,26 @@ void AdbConnectionState::StopDebuggerThreads() {
   shutting_down_ = true;
   // Wakeup the poll loop.
   uint64_t data = 1;
-  TEMP_FAILURE_RETRY(write(sleep_event_fd_, &data, sizeof(data)));
+  if (sleep_event_fd_ != -1) {
+    TEMP_FAILURE_RETRY(write(sleep_event_fd_, &data, sizeof(data)));
+  }
 }
 
 // The plugin initialization function.
 extern "C" bool ArtPlugin_Initialize() REQUIRES_SHARED(art::Locks::mutator_lock_) {
   DCHECK(art::Runtime::Current()->GetJdwpProvider() == art::JdwpProvider::kAdbConnection);
   // TODO Provide some way for apps to set this maybe?
+  DCHECK(gState == nullptr);
   gState = new AdbConnectionState(kDefaultJdwpAgentName);
-  CHECK(gState != nullptr);
   return ValidateJdwpOptions(art::Runtime::Current()->GetJdwpOptions());
 }
 
 extern "C" bool ArtPlugin_Deinitialize() {
-  CHECK(gState != nullptr);
-  // Just do this a second time?
-  // TODO I don't think this should be needed.
   gState->StopDebuggerThreads();
-  delete gState;
+  if (!gState->DebuggerThreadsStarted()) {
+    // If debugger threads were started then those threads will delete the state once they are done.
+    delete gState;
+  }
   return true;
 }
 
diff --git a/adbconnection/adbconnection.h b/adbconnection/adbconnection.h
index e63a3b607d..04e39bf4ff 100644
--- a/adbconnection/adbconnection.h
+++ b/adbconnection/adbconnection.h
@@ -72,7 +72,7 @@ struct AdbConnectionDdmCallback : public art::DdmCallback {
 
 class AdbConnectionState {
  public:
-  explicit AdbConnectionState(const std::string& agent_name);
+  explicit AdbConnectionState(const std::string& name);
 
   // Called on the listening thread to start dealing with new input. thr is used to attach the new
   // thread to the runtime.
@@ -85,6 +85,11 @@ class AdbConnectionState {
   // Stops debugger threads during shutdown.
   void StopDebuggerThreads();
 
+  // If StartDebuggerThreads was called successfully.
+  bool DebuggerThreadsStarted() {
+    return started_debugger_threads_;
+  }
+
  private:
   uint32_t NextDdmId();
 
@@ -161,6 +166,8 @@ class AdbConnectionState {
 
   std::atomic<uint32_t> next_ddm_id_;
 
+  bool started_debugger_threads_;
+
   socklen_t control_addr_len_;
   union {
     sockaddr_un controlAddrUn;