Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit cadab3b8 authored by David Anderson's avatar David Anderson
Browse files

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
parent 8376d87e

fs_mgr/libsnapshot/snapuserd_server.cpp

+32 −17
Original line number Original line Diff line number Diff line
@@ -77,9 +77,8 @@ void SnapuserdServer::ShutdownThreads() { 
    JoinAllThreads();

    JoinAllThreads();

}

}





const std::string& DmUserHandler::GetMiscName() const {

DmUserHandler::DmUserHandler(std::unique_ptr<Snapuserd>&& snapuserd)

    return snapuserd_->GetMiscName();

    : snapuserd_(std::move(snapuserd)), misc_name_(snapuserd_->GetMiscName()) {}

}





bool SnapuserdServer::Sendmsg(android::base::borrowed_fd fd, const std::string& msg) {

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));

    ssize_t ret = TEMP_FAILURE_RETRY(send(fd.get(), msg.data(), msg.size(), 0));
@@ -148,7 +147,7 @@ bool SnapuserdServer::Receivemsg(android::base::borrowed_fd fd, const std::strin 
                LOG(ERROR) << "Could not find handler: " << out[1];

                LOG(ERROR) << "Could not find handler: " << out[1];

                return Sendmsg(fd, "fail");

                return Sendmsg(fd, "fail");

            }

            }

            if ((*iter)->snapuserd()->IsAttached()) {

            if (!(*iter)->snapuserd() || (*iter)->snapuserd()->IsAttached()) {

                LOG(ERROR) << "Tried to re-attach control device: " << out[1];

                LOG(ERROR) << "Tried to re-attach control device: " << out[1];

                return Sendmsg(fd, "fail");

                return Sendmsg(fd, "fail");

            }

            }
@@ -185,7 +184,7 @@ bool SnapuserdServer::Receivemsg(android::base::borrowed_fd fd, const std::strin 
                LOG(ERROR) << "Malformed delete message, " << out.size() << " parts";

                LOG(ERROR) << "Malformed delete message, " << out.size() << " parts";

                return Sendmsg(fd, "fail");

                return Sendmsg(fd, "fail");

            }

            }

            if (!RemoveHandler(out[1], true)) {

            if (!RemoveAndJoinHandler(out[1])) {

                return Sendmsg(fd, "fail");

                return Sendmsg(fd, "fail");

            }

            }

            return Sendmsg(fd, "success");

            return Sendmsg(fd, "success");
@@ -203,20 +202,38 @@ bool SnapuserdServer::Receivemsg(android::base::borrowed_fd fd, const std::strin 
}

}





void SnapuserdServer::RunThread(std::shared_ptr<DmUserHandler> handler) {

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()) {

    while (!StopRequested()) {

        if (!handler->snapuserd()->Run()) {

        if (!handler->snapuserd()->Run()) {

            LOG(INFO) << "Snapuserd: Thread terminating";

            break;

            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.

        std::lock_guard<std::mutex> lock(lock_);

    RemoveHandler(handler->GetMiscName(), false);

        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) {

bool SnapuserdServer::Start(const std::string& socketname) {
@@ -351,7 +368,7 @@ bool SnapuserdServer::StartHandler(const std::shared_ptr<DmUserHandler>& handler 
    CHECK(!handler->snapuserd()->IsAttached());

    CHECK(!handler->snapuserd()->IsAttached());





    if (!handler->snapuserd()->InitBackingAndControlDevice()) {

    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;

        return false;

    }

    }
@@ -364,14 +381,14 @@ auto SnapuserdServer::FindHandler(std::lock_guard<std::mutex>* proof_of_lock, 
    CHECK(proof_of_lock);

    CHECK(proof_of_lock);





    for (auto iter = dm_users_.begin(); iter != dm_users_.end(); iter++) {

    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 iter;

        }

        }

    }

    }

    return dm_users_.end();

    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::shared_ptr<DmUserHandler> handler;

    {

    {

        std::lock_guard<std::mutex> lock(lock_);

        std::lock_guard<std::mutex> lock(lock_);
@@ -386,10 +403,8 @@ bool SnapuserdServer::RemoveHandler(const std::string& misc_name, bool wait) { 
    }

    }





    auto& th = handler->thread();

    auto& th = handler->thread();

    if (th.joinable() && wait) {

    if (th.joinable()) {

        th.join();

        th.join();

    } else if (handler->snapuserd()->IsAttached()) {

        th.detach();

    }

    }

    return true;

    return true;

}

}

fs_mgr/libsnapshot/snapuserd_server.h

+11 −9
Original line number Original line Diff line number Diff line
@@ -46,18 +46,19 @@ enum class DaemonOperations { 
};

};





class DmUserHandler {

class DmUserHandler {

  private:

    std::thread thread_;

    std::unique_ptr<Snapuserd> snapuserd_;



  public:

  public:

    explicit DmUserHandler(std::unique_ptr<Snapuserd>&& snapuserd)

    explicit DmUserHandler(std::unique_ptr<Snapuserd>&& snapuserd);

        : snapuserd_(std::move(snapuserd)) {}





    void FreeResources() { snapuserd_ = nullptr; }

    const std::unique_ptr<Snapuserd>& snapuserd() const { return snapuserd_; }

    const std::unique_ptr<Snapuserd>& snapuserd() const { return snapuserd_; }

    std::thread& thread() { return thread_; }

    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 {

class Stoppable {
@@ -71,8 +72,9 @@ class Stoppable { 




    bool StopRequested() {

    bool StopRequested() {

        // checks if value in future object is available

        // 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 false;

        }

        return true;

        return true;

    }

    }

    // Request the thread to stop by setting value in promise object

    // Request the thread to stop by setting value in promise object
@@ -98,7 +100,7 @@ class SnapuserdServer : public Stoppable { 
    bool Receivemsg(android::base::borrowed_fd fd, const std::string& str);

    bool Receivemsg(android::base::borrowed_fd fd, const std::string& str);





    void ShutdownThreads();

    void ShutdownThreads();

    bool RemoveHandler(const std::string& control_device, bool wait);

    bool RemoveAndJoinHandler(const std::string& control_device);

    DaemonOperations Resolveop(std::string& input);

    DaemonOperations Resolveop(std::string& input);

    std::string GetDaemonStatus();

    std::string GetDaemonStatus();

    void Parsemsg(std::string const& msg, const char delim, std::vector<std::string>& out);

    void Parsemsg(std::string const& msg, const char delim, std::vector<std::string>& out);