libsnapshot: Fix a race condition in WaitForDelete.
WaitForDelete is supposed to block until close() has been called on the
COW image. However, it could race with the destructor for Snapuserd
since nothing guaranteed it was freed within the global lock.
This patch fixes the bug and refactors the surrounding code to make the
responsibilities of each thread clearer.
Bug: N/A
Test: vts_libsnapshot_test
Change-Id: Icfc264e6dff378db585c81cde381cc24269f4800
diff --git a/fs_mgr/libsnapshot/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd_server.cpp
index 8351155..38abaec 100644
--- a/fs_mgr/libsnapshot/snapuserd_server.cpp
+++ b/fs_mgr/libsnapshot/snapuserd_server.cpp
@@ -77,9 +77,8 @@
JoinAllThreads();
}
-const std::string& DmUserHandler::GetMiscName() const {
- return snapuserd_->GetMiscName();
-}
+DmUserHandler::DmUserHandler(std::unique_ptr<Snapuserd>&& snapuserd)
+ : snapuserd_(std::move(snapuserd)), misc_name_(snapuserd_->GetMiscName()) {}
bool SnapuserdServer::Sendmsg(android::base::borrowed_fd fd, const std::string& msg) {
ssize_t ret = TEMP_FAILURE_RETRY(send(fd.get(), msg.data(), msg.size(), 0));
@@ -148,7 +147,7 @@
LOG(ERROR) << "Could not find handler: " << out[1];
return Sendmsg(fd, "fail");
}
- if ((*iter)->snapuserd()->IsAttached()) {
+ if (!(*iter)->snapuserd() || (*iter)->snapuserd()->IsAttached()) {
LOG(ERROR) << "Tried to re-attach control device: " << out[1];
return Sendmsg(fd, "fail");
}
@@ -185,7 +184,7 @@
LOG(ERROR) << "Malformed delete message, " << out.size() << " parts";
return Sendmsg(fd, "fail");
}
- if (!RemoveHandler(out[1], true)) {
+ if (!RemoveAndJoinHandler(out[1])) {
return Sendmsg(fd, "fail");
}
return Sendmsg(fd, "success");
@@ -203,20 +202,38 @@
}
void SnapuserdServer::RunThread(std::shared_ptr<DmUserHandler> handler) {
- LOG(INFO) << "Entering thread for handler: " << handler->GetMiscName();
+ LOG(INFO) << "Entering thread for handler: " << handler->misc_name();
while (!StopRequested()) {
if (!handler->snapuserd()->Run()) {
- LOG(INFO) << "Snapuserd: Thread terminating";
break;
}
}
- LOG(INFO) << "Exiting thread for handler: " << handler->GetMiscName();
+ auto misc_name = handler->misc_name();
+ LOG(INFO) << "Handler thread about to exit: " << misc_name;
- // If the main thread called RemoveHandler, the handler was already removed
- // from within the lock, and calling RemoveHandler again has no effect.
- RemoveHandler(handler->GetMiscName(), false);
+ {
+ std::lock_guard<std::mutex> lock(lock_);
+ auto iter = FindHandler(&lock, handler->misc_name());
+ if (iter == dm_users_.end()) {
+ // RemoveAndJoinHandler() already removed us from the list, and is
+ // now waiting on a join(), so just return.
+ LOG(INFO) << "Exiting handler thread to allow for join: " << misc_name;
+ return;
+ }
+
+ LOG(INFO) << "Exiting handler thread and freeing resources: " << misc_name;
+
+ if (handler->snapuserd()->IsAttached()) {
+ handler->thread().detach();
+ }
+
+ // Important: free resources within the lock. This ensures that if
+ // WaitForDelete() is called, the handler is either in the list, or
+ // it's not and its resources are guaranteed to be freed.
+ handler->FreeResources();
+ }
}
bool SnapuserdServer::Start(const std::string& socketname) {
@@ -351,7 +368,7 @@
CHECK(!handler->snapuserd()->IsAttached());
if (!handler->snapuserd()->InitBackingAndControlDevice()) {
- LOG(ERROR) << "Failed to initialize control device: " << handler->GetMiscName();
+ LOG(ERROR) << "Failed to initialize control device: " << handler->misc_name();
return false;
}
@@ -364,14 +381,14 @@
CHECK(proof_of_lock);
for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) {
- if ((*iter)->GetMiscName() == misc_name) {
+ if ((*iter)->misc_name() == misc_name) {
return iter;
}
}
return dm_users_.end();
}
-bool SnapuserdServer::RemoveHandler(const std::string& misc_name, bool wait) {
+bool SnapuserdServer::RemoveAndJoinHandler(const std::string& misc_name) {
std::shared_ptr<DmUserHandler> handler;
{
std::lock_guard<std::mutex> lock(lock_);
@@ -386,10 +403,8 @@
}
auto& th = handler->thread();
- if (th.joinable() && wait) {
+ if (th.joinable()) {
th.join();
- } else if (handler->snapuserd()->IsAttached()) {
- th.detach();
}
return true;
}
diff --git a/fs_mgr/libsnapshot/snapuserd_server.h b/fs_mgr/libsnapshot/snapuserd_server.h
index 01e2365..7cbc2de 100644
--- a/fs_mgr/libsnapshot/snapuserd_server.h
+++ b/fs_mgr/libsnapshot/snapuserd_server.h
@@ -46,18 +46,19 @@
};
class DmUserHandler {
- private:
- std::thread thread_;
- std::unique_ptr<Snapuserd> snapuserd_;
-
public:
- explicit DmUserHandler(std::unique_ptr<Snapuserd>&& snapuserd)
- : snapuserd_(std::move(snapuserd)) {}
+ explicit DmUserHandler(std::unique_ptr<Snapuserd>&& snapuserd);
+ void FreeResources() { snapuserd_ = nullptr; }
const std::unique_ptr<Snapuserd>& snapuserd() const { return snapuserd_; }
std::thread& thread() { return thread_; }
- const std::string& GetMiscName() const;
+ const std::string& misc_name() const { return misc_name_; }
+
+ private:
+ std::thread thread_;
+ std::unique_ptr<Snapuserd> snapuserd_;
+ std::string misc_name_;
};
class Stoppable {
@@ -71,8 +72,9 @@
bool StopRequested() {
// checks if value in future object is available
- if (futureObj_.wait_for(std::chrono::milliseconds(0)) == std::future_status::timeout)
+ if (futureObj_.wait_for(std::chrono::milliseconds(0)) == std::future_status::timeout) {
return false;
+ }
return true;
}
// Request the thread to stop by setting value in promise object
@@ -98,7 +100,7 @@
bool Receivemsg(android::base::borrowed_fd fd, const std::string& str);
void ShutdownThreads();
- bool RemoveHandler(const std::string& control_device, bool wait);
+ bool RemoveAndJoinHandler(const std::string& control_device);
DaemonOperations Resolveop(std::string& input);
std::string GetDaemonStatus();
void Parsemsg(std::string const& msg, const char delim, std::vector<std::string>& out);