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
diff --git a/adbconnection/adbconnection.cc b/adbconnection/adbconnection.cc
index d8db923..634d6b5 100644
--- a/adbconnection/adbconnection.cc
+++ b/adbconnection/adbconnection.cc
@@ -139,7 +139,8 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
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;
}