Don't use allocations during signaling 89/315889/3
authorKrzysztof Jackiewicz <k.jackiewicz@samsung.com>
Thu, 5 Dec 2024 16:26:59 +0000 (17:26 +0100)
committerKrzysztof Jackiewicz <k.jackiewicz@samsung.com>
Fri, 6 Dec 2024 19:12:47 +0000 (20:12 +0100)
This is to prevent deadlock in malloc. It may happen if a supplementary
thread is interrupted after it took an internal glibc lock in malloc()
and it waits in a signal handler for the main thread. The main thread
won't be able to allocate memory due to lock taken by supplementary
thread.

Change-Id: I218075c2c2d6befa8fafb141e0507e64b5b47406

src/client/CMakeLists.txt
src/client/check-proper-drop.cpp [deleted file]
src/client/client-security-manager.cpp
src/common/include/check-proper-drop.h
test/CMakeLists.txt
test/test_misc.cpp

index c8db9388eaadb5b105fbc4fde308af8d22747cd3..bea9571783c8caa6e0c4da16c384a18fdaf28a06 100644 (file)
@@ -49,7 +49,6 @@ SET(CLIENT_SOURCES
     ${CLIENT_PATH}/client-common.cpp
     ${CLIENT_PATH}/client-offline.cpp
     ${CLIENT_PATH}/client-label-monitor.cpp
-    ${CLIENT_PATH}/check-proper-drop.cpp
     )
 
 LINK_DIRECTORIES(${CLIENT_DEP_LIBRARY_DIRS})
diff --git a/src/client/check-proper-drop.cpp b/src/client/check-proper-drop.cpp
deleted file mode 100644 (file)
index 0f37436..0000000
+++ /dev/null
@@ -1,75 +0,0 @@
-/*
- * Copyright (c) 2016-2023 Samsung Electronics Co., Ltd. All rights reserved.
- *
- * This file is licensed under the terms of MIT License or the Apache License
- * Version 2.0 of your choice. See the LICENSE.MIT file for MIT license details.
- * See the LICENSE file or the notice below for Apache License Version 2.0
- * details.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-/*
- * @brief Implementation of proper privilege dropping check utilities, client side
- */
-
-#include <string>
-
-#include <check-proper-drop.h>
-#include <dpl/exception.h>
-#include <dpl/log/log.h>
-#include <filesystem.h>
-
-namespace SecurityManager {
-namespace CheckProperDrop {
-
-namespace {
-
-DECLARE_EXCEPTION_TYPE(SecurityManager::Exception, DropError)
-
-}  // namespace
-
-std::unordered_set<int> checkThreads(int currentTid)
-{
-    FS::FileNameVector current_tids;
-    for (unsigned i = 0; i < 10; ++i) {
-        try {
-            current_tids = FS::getSubDirectoriesFromDirectory("/proc/self/task", true);
-            break;
-        } catch(const FS::Exception::FileError&) {
-            if (i == 9)
-                throw;
-
-            /*
-             *  This may happen if a thread disappears, an entry is removed from /proc and fstatat
-             *  fails.
-             */
-            LogWarning("Reading /proc/self/task/ failed. Retrying.");
-        }
-    }
-
-    std::unordered_set<int> result;
-    for (auto &tid_s : current_tids) {
-        int tid = atoi(tid_s.c_str());
-
-        if (tid == currentTid)
-            continue;
-
-        LogDebug("Thread " << tid_s << " new");
-        result.emplace(tid);
-    }
-    return result;
-}
-
-}  // namespace CheckProperDrop
-}  // namespace SecurityManager
index ab2ed3af476407de9b109ab3d9ce2ae72966de76..1856842819e6af947ea77be11781853000e3a0c7 100644 (file)
@@ -102,6 +102,8 @@ static std::atomic<int> g_th_barrier;
 static std::atomic<int> g_thread_state[MAX_THREAD_COUNT];
 static std::atomic<int> g_thread_index[MAX_THREAD_COUNT];
 static std::atomic<int> g_managed_tids_num;
+static bool g_thread_alive[MAX_THREAD_COUNT];
+static bool g_thread_signaled[MAX_THREAD_COUNT];
 static cap_t g_cap;
 
 static inline void add_managed_tid(int tid){
@@ -111,6 +113,7 @@ static inline void add_managed_tid(int tid){
     }
     g_thread_state[g_managed_tids_num] = 0;
     g_thread_index[g_managed_tids_num] = tid;
+    g_thread_alive[g_managed_tids_num] = true;
     g_managed_tids_num++;
 }
 
@@ -132,14 +135,6 @@ static inline void set_tid_state(int tid, int state = 0) {
     g_thread_state[index] = state;
 }
 
-static int count_alive_tids_with_state(int status, const std::unordered_set<int>& alive_tids) {
-    int count = 0;
-    for (int i = 0; i < g_managed_tids_num; ++i)
-        if (g_thread_state[i] == status && alive_tids.find(g_thread_index[i]) != alive_tids.end())
-            ++ count;
-    return count;
-}
-
 SECURITY_MANAGER_API
 const char *security_manager_strerror(enum lib_retcode rc)
 {
@@ -645,6 +640,175 @@ static inline void security_manager_pre_check()
     cap_free(my_caps);
 }
 
+static bool alive_threads_have_state(int status) noexcept
+{
+    for (int i = 0; i < g_managed_tids_num; ++i) {
+        if (!g_thread_alive[i])
+            continue;
+
+        if (g_thread_state[i] != status)
+            return false;
+    }
+    return true;
+}
+
+static int get_alive_threads(int own_tid) noexcept
+{
+    // reset alive status
+    memset(&g_thread_alive, 0, sizeof(g_thread_alive));
+
+    auto dir_fd = open("/proc/self/task", O_DIRECTORY | O_CLOEXEC);
+    if (dir_fd == -1)
+        return errno;
+
+    char buf[1024];
+    struct dirent64 *d;
+    int bpos = 0;
+    for(;;) {
+        ssize_t nread = getdents64(dir_fd, buf, sizeof(buf));
+        if (nread == -1) {
+            close(dir_fd);
+            return errno;
+        }
+
+        if (nread == 0)
+            break;
+
+        for (bpos = 0; bpos < nread; bpos += d->d_reclen) {
+            d = (struct dirent64 *) (buf + bpos);
+
+            // TODO it's DT_UNKNOWN for some reason
+//            d_type = *(buf + bpos + d->d_reclen - 1);
+//            if (d_type != DT_DIR)
+//                continue;
+
+            if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0)
+                continue;
+
+            char* endptr;
+
+            errno = 0;
+            long tid = strtol(d->d_name, &endptr, 10);
+            if (errno != 0) {
+                close(dir_fd);
+                return errno;
+            }
+
+            if (tid == own_tid)
+                continue;
+
+            // Mark as alive. Append new if necessary.
+            int i = 0;
+            for (; i < g_managed_tids_num; ++i) {
+                if (g_thread_index[i] == tid) {
+                    g_thread_alive[i] = true;
+                    break;
+                }
+            }
+            if (i != g_managed_tids_num)
+                continue;
+
+            add_managed_tid(tid);
+        }
+    }
+
+    close(dir_fd);
+    return 0;
+}
+
+static int check_threads(int own_tid) noexcept
+{
+    for (unsigned i = 0; i < 10; ++i) {
+        auto ret = get_alive_threads(own_tid);
+        if (ret == 0)
+            break;
+
+        /*
+         *  This may happen if a thread disappears, an entry is removed from /proc and fstatat
+         *  fails.
+         */
+        if (i == 9)
+            return ret;
+    }
+
+    return 0;
+}
+
+static bool no_new_threads() noexcept
+{
+    for (int i = 0; i < g_managed_tids_num; ++i) {
+        if (!g_thread_alive[i])
+            continue;
+
+        if (!g_thread_signaled[i])
+            return false;
+    }
+    return true;
+}
+
+static int signal_and_wait_for_handlers(pid_t own_pid, int own_tid) noexcept
+{
+       // No allocations allowed in this function
+       int ret = 0;
+    int time_left = MAX_SIG_WAIT_TIME;
+    do {
+        ret = check_threads(own_tid);
+        if (ret != 0)
+            break;
+
+        for (int i = 0; i < g_managed_tids_num; ++i) {
+            if (!g_thread_alive[i])
+                break;
+
+            if (!g_thread_signaled[i]) {
+                g_thread_signaled[i] = true;
+                int tid = g_thread_index[i];
+
+                // thread not signaled yet, send signal
+                if (Syscall::tgkill(own_pid, tid, SIGSETXID) < 0) {
+                    auto err = errno;
+                    if (EAGAIN == err) { // resource temporarily unavailable, trying again
+                        usleep(SLEEP_CONST);   // 10 ms
+                        if (Syscall::tgkill(own_pid, tid, SIGSETXID) < 0) {
+                            err = errno;
+                            if (ESRCH == err) { // thread already gone - noop
+                                continue;
+                            } else {
+                                return err;
+                            }
+                        }
+                    } else if (ESRCH == err) { // thread already gone - noop
+                        continue;
+                    } else {
+                        return err;
+                    }
+                }
+            }
+        }
+
+        usleep(SLEEP_CONST);   // 10 ms
+        --time_left;
+
+        // break if number of threads in waiting state equals to number of alive tids minus current one
+        if (alive_threads_have_state(1)) {
+            // threads that were read some lines above are all sleeping here - BUT - could have
+            // spawned new threads between reading list from /proc and checking status with count_alive_tids_with_state()
+            // to make sure the loop can end, here we read /proc again
+            ret = check_threads(own_tid);
+            if (ret != 0)
+                break;
+
+            if (no_new_threads())
+                // here indeed, no new threads are present and they are all waiting in the signal handler
+                break;
+        }
+
+        if (time_left == 0)
+            return ETIME;
+    } while (1);
+    return ret;
+}
+
 static inline int security_manager_sync_threads_internal(const std::string &app_label)
 {
     static_assert(ATOMIC_INT_LOCK_FREE == 2, "std::atomic<int> is not always lock free");
@@ -653,8 +817,6 @@ static inline int security_manager_sync_threads_internal(const std::string &app_
     int own_tid = Syscall::gettid();
     const pid_t own_pid = getpid();
 
-    std::unordered_set<int> tids_sent_signals = {};
-
     g_cap = cap_init();
 
     if (!g_cap) {
@@ -700,65 +862,12 @@ static inline int security_manager_sync_threads_internal(const std::string &app_
     std::atomic_thread_fence(std::memory_order_release);
 
     g_th_barrier = 0;
-    int time_left = MAX_SIG_WAIT_TIME;
-    std::unordered_set<int> current_tids = {};
 
-    do {
-        current_tids = CheckProperDrop::checkThreads(own_tid);
-        for (auto existing_tid : current_tids) {
-            if (tids_sent_signals.find(existing_tid) == tids_sent_signals.end()) {
-                tids_sent_signals.insert(existing_tid);
-                add_managed_tid(existing_tid);
-                // thread not managed yet, send signal
-                if (Syscall::tgkill(own_pid, existing_tid, SIGSETXID) < 0) {
-                    auto err = errno;
-                    if (EAGAIN == err) { // resource temporarily unavailable, trying again
-                        LogWarning("Received EAGAIN from tgkill, wait a bit & try again");
-                        usleep(SLEEP_CONST);   // 10 ms
-                        if (Syscall::tgkill(own_pid, existing_tid, SIGSETXID) < 0) {
-                            err = errno;
-                            if (ESRCH == err) { // thread already gone - noop
-                                continue;
-                            } else {
-                                LogWithErrno(err, "tgkill()");
-                                abort();
-                            }
-                        }
-                    } else if (ESRCH == err) { // thread already gone - noop
-                        continue;
-                    } else {
-                        LogWithErrno(err, "tgkill()");
-                        abort();
-                    }
-                }
-            }
-        }
-
-        usleep(SLEEP_CONST);   // 10 ms
-        --time_left;
-
-        // break if number of threads in waiting state equals to number of alive tids minus current one
-        int ready_tids = count_alive_tids_with_state(1, current_tids);
-        if (ready_tids == (int)current_tids.size()) {
-            // threads that were read some lines above are all sleeping here - BUT - could have
-            // spawned new threads between reading list from /proc and checking status with count_alive_tids_with_state()
-            // to make sure the loop can end, here we read /proc again
-            if(CheckProperDrop::checkThreads(own_tid) == current_tids)
-                // here indeed, no new threads are present and they are all waiting in the signal handler
-                break;
-        }
-
-        if (time_left % 500 == 0)
-            LogWarning("Still waiting for threads to receive signals... signals received by: " << ready_tids
-                                                                                               << ", all tids alive now without current: "
-                                                                                               << current_tids.size());
-        if (time_left == 0) {
-            LogError("Too much waiting for receiving signals, aborting! Signals received by: " << ready_tids
-                                                                                               << ", out of alive tids without current: "
-                                                                                               << current_tids.size());
-            abort();
-        }
-    } while (1);
+    int ret = signal_and_wait_for_handlers(own_pid, own_tid);
+    if (ret != 0) {
+        LogError("Error occured during signaling: " << ret);
+        abort();
+    }
 
     // here, all TIDs except current one are waiting to start changing attributes
     // We can assume these TIDs will continue to live (no need to read /proc again), since no logic
@@ -766,25 +875,21 @@ static inline int security_manager_sync_threads_internal(const std::string &app_
 
     g_th_barrier++; // this starts the signal handlers - they will proceed once they wake up
 
-    int ready_tids = 0;
-
+    bool ready = false;
     for (int i = MAX_SIG_WAIT_TIME; i > 0; --i) {
         // break if number of completed tids equals to all of them minus current one!
-        ready_tids = count_alive_tids_with_state(2, current_tids);
-        if (ready_tids == (int)current_tids.size())
+
+        ready = alive_threads_have_state(2);
+        if (ready)
             break;
         usleep(SLEEP_CONST);   // 10 ms
         if (i % 500 == 0)
-            LogWarning("Still waiting for threads to finalize handlers... completed by: " << ready_tids
-                                                                                          << ", all tids managed now: "
-                                                                                          << current_tids.size());
+            LogWarning("Still waiting for threads to finalize handlers.");
     }
 
-    if (ready_tids != (int)current_tids.size()) {
+    if (!ready) {
         // not all TIDs reached this stage, aborting!
-        LogError("Too much waiting for sig handler completion, aborting! Signals completed by: " << ready_tids
-                                                                                                 << ", all tids managed now: "
-                                                                                                 << current_tids.size());
+        LogError("Too much waiting for sig handler completion, aborting!");
         abort();
     }
 
index 03e073454f273a921f9a7b91126f6538c3432e96..061b664cccfe6e6eaa7f3bb7dd0e82461c9e4fd6 100644 (file)
@@ -26,7 +26,6 @@
 #pragma once
 
 #include <utils.h>
-#include <unordered_set>
 
 namespace SecurityManager {
 namespace CheckProperDrop {
@@ -45,10 +44,5 @@ static_assert(N_FLAG_BITS <= 7, "flags too large for int8_t");
 // Otherwise bit i set iff NS_NAMES[i] checked.
 int8_t computeFlags();
 
-
-// CLIENT ONLY
-// returns threads TIDS
-std::unordered_set<int> checkThreads(int currentTid);
-
 }  // namespace CheckProperDrop
 }  // namespace SecurityManager
index 34029cb6677b47b5c3c6bbdb79d0be18f9bc2a88..14444a6bbc0fc8d4c5001c784e7dc5392f27e100 100644 (file)
@@ -126,7 +126,6 @@ SET(SM_TESTS_SOURCES
     ${PROJECT_SOURCE_DIR}/src/common/filesystem.cpp
     ${PROJECT_SOURCE_DIR}/src/common/tzplatform-config.cpp
     ${PROJECT_SOURCE_DIR}/src/common/utils.cpp
-    ${PROJECT_SOURCE_DIR}/src/client/check-proper-drop.cpp
     ${GEN_PATH}/db.h
 )
 
index d536b122ce4bdd8cf20d7091bf8822eb6c692486..c24bbe16e1a175f1d22fe8abac7d2890085f580a 100644 (file)
@@ -27,7 +27,6 @@
  */
 
 #include <algorithm>
-#include <check-proper-drop.h>
 #include <credentials.h>
 #include <dpl/errno_string.h>
 #include <dpl/exception.h>
@@ -211,13 +210,6 @@ POSITIVE_TEST_CASE(T293_possiblyUnterminatedArrayToString)
     BOOST_REQUIRE_EQUAL("", possiblyUnterminatedArrayToString(a));
 }
 
-POSITIVE_TEST_CASE(T294_checkThreads)
-{
-    pid_t myTid = gettid();
-    auto tids = CheckProperDrop::checkThreads(myTid);
-    BOOST_REQUIRE_MESSAGE(tids.count(myTid) == 0,  "CheckProperDrop::checkThreads includes own tid");
-}
-
 POSITIVE_TEST_CASE(T295_GetErrnoString)
 {
     std::string str = GetErrnoString(EINVAL);