diff options
author | Alex Light <allight@google.com> | 2018-01-25 11:26:28 -0800 |
---|---|---|
committer | Alex Light <allight@google.com> | 2018-02-01 13:52:53 -0800 |
commit | d6f9d8583f1f8791456d89e9252d8fa0374b69bb (patch) | |
tree | f491bfa983265c42f4594c04770fdee0eba6a712 /adbconnection/adbconnection.cc | |
parent | 92d0c8b68c24a2fa21f95d63a1ff2fb00fdb9aaf (diff) | |
download | platform_art-d6f9d8583f1f8791456d89e9252d8fa0374b69bb.tar.gz platform_art-d6f9d8583f1f8791456d89e9252d8fa0374b69bb.tar.bz2 platform_art-d6f9d8583f1f8791456d89e9252d8fa0374b69bb.zip |
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
Diffstat (limited to 'adbconnection/adbconnection.cc')
-rw-r--r-- | adbconnection/adbconnection.cc | 25 |
1 files changed, 18 insertions, 7 deletions
diff --git a/adbconnection/adbconnection.cc b/adbconnection/adbconnection.cc index d8db923a289..634d6b56dfa 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; } |