Code review fixes: safety, performance, and correctness

.github/workflows/cmake-multi-platform.yml

@@ -116,17 +116,18 @@ jobs:
       uses: vmactions/solaris-vm@v1
       with:
         prepare: |
-          pkg install developer/gcc
-          pkg install developer/build/cmake
-          pkg install developer/build/gnu-make
-          pkg install system/header
-          pkg install x11/header
-          pkg install x11/library/libx11
-          pkg install x11/library/libxrandr
-          pkg install x11/library/libxinerama
-          pkg install x11/library/libxcursor
-          pkg install x11/library/libxi
-          pkg install library/mesa
+          pkg install developer/gcc || true
+          pkg install developer/versioning/git || true
+          pkg install developer/build/cmake || true
+          pkg install developer/build/gnu-make || true
+          pkg install system/header || true
+          pkg install x11/header || true
+          pkg install x11/library/libx11 || true
+          pkg install x11/library/libxrandr || true
+          pkg install x11/library/libxinerama || true
+          pkg install x11/library/libxcursor || true
+          pkg install x11/library/libxi || true
+          pkg install library/mesa || true
         run: |
           export PATH="/usr/gcc/*/bin:$PATH"
           cmake -B build -DCMAKE_BUILD_TYPE=Release -DBUILD_NCURSES=ON

CMakeLists.txt

@@ -240,8 +240,8 @@ if(NOT NCURSES_FOUND AND BUILD_TUI AND BUILD_NCURSES)
             --without-ada
             --enable-widec
             --disable-stripping
-        BUILD_COMMAND make
-        INSTALL_COMMAND make install
+        BUILD_COMMAND ${CMAKE_MAKE_PROGRAM}
+        INSTALL_COMMAND ${CMAKE_MAKE_PROGRAM} install
         BUILD_IN_SOURCE 1
     )
 

src/core/format_utils.hpp

@@ -1,8 +1,7 @@
 #pragma once
 
 #include <cstdint>
-#include <sstream>
-#include <iomanip>
+#include <format>
 #include <string>
 
 namespace pex {
@@ -26,17 +25,15 @@ inline std::string format_bytes(int64_t bytes, bool compact = true) {
         unit_idx++;
     }
 
-    std::ostringstream oss;
     if (unit_idx == 0) {
-        oss << bytes << sep << units[unit_idx];
+        return std::format("{}{}{}", bytes, sep, units[unit_idx]);
     } else if (size >= 100.0) {
-        oss << std::fixed << std::setprecision(0) << size << sep << units[unit_idx];
+        return std::format("{:.0f}{}{}", size, sep, units[unit_idx]);
     } else if (size >= 10.0) {
-        oss << std::fixed << std::setprecision(1) << size << sep << units[unit_idx];
+        return std::format("{:.1f}{}{}", size, sep, units[unit_idx]);
     } else {
-        oss << std::fixed << std::setprecision(2) << size << sep << units[unit_idx];
+        return std::format("{:.2f}{}{}", size, sep, units[unit_idx]);
     }
-    return oss.str();
 }
 
 } // namespace pex

src/core/services/data_store.cpp

@@ -2,6 +2,7 @@
 #include <algorithm>
 #include <ranges>
 #include <set>
+#include <unordered_map>
 
 namespace pex {
 
@@ -166,7 +167,7 @@ void DataStore::collect_data() {
     });
 
     // Build process tree
-    std::map<int, std::unique_ptr<ProcessNode>> nodes;
+    std::unordered_map<int, std::unique_ptr<ProcessNode>> nodes;
     for (auto& proc : processes) {
         auto node = std::make_unique<ProcessNode>();
         node->info = std::move(proc);
@@ -183,16 +184,16 @@ void DataStore::collect_data() {
     }
 
     // Build children map
-    std::map<int, std::vector<int>> children_map;
+    std::unordered_map<int, std::vector<int>> children_map;
     for (auto& [pid, node] : nodes) {
         if (int ppid = node->info.parent_pid; ppid != pid && nodes.contains(ppid)) {
             children_map[ppid].push_back(pid);
         }
     }
 
     // Recursive function to attach children
-    std::function<void(ProcessNode*, std::map<int, std::unique_ptr<ProcessNode>>&)> attach_children;
-    attach_children = [&](ProcessNode* parent, std::map<int, std::unique_ptr<ProcessNode>>& all_nodes) {
+    std::function<void(ProcessNode*, std::unordered_map<int, std::unique_ptr<ProcessNode>>&)> attach_children;
+    attach_children = [&](ProcessNode* parent, std::unordered_map<int, std::unique_ptr<ProcessNode>>& all_nodes) {
         if (const auto it = children_map.find(parent->info.pid); it != children_map.end()) {
             for (int child_pid : it->second) {
                 if (auto child_it = all_nodes.find(child_pid); child_it != all_nodes.end()) {
@@ -324,7 +325,7 @@ void DataStore::calculate_tree_totals(ProcessNode& node) {
     }
 }
 
-void DataStore::build_process_map(ProcessNode* node, std::map<int, ProcessNode*>& map) {
+void DataStore::build_process_map(ProcessNode* node, std::unordered_map<int, ProcessNode*>& map) {
     map[node->info.pid] = node;
     for (auto& child : node->children) {
         build_process_map(child.get(), map);

src/core/services/data_store.hpp

@@ -6,7 +6,7 @@
 #include "../model/errors.hpp"
 #include "../model/system_info.hpp"
 #include <vector>
-#include <map>
+#include <unordered_map>
 #include <memory>
 #include <thread>
 #include <mutex>
@@ -38,7 +38,7 @@ struct ProcessNode {
 // are shared via shared_ptr (never modified after construction).
 struct DataSnapshot {
     std::vector<std::unique_ptr<ProcessNode>> process_tree;
-    std::map<int, ProcessNode*> process_map;  // Non-owning pointers into process_tree
+    std::unordered_map<int, ProcessNode*> process_map;  // Non-owning pointers into process_tree
 
     // System stats
     int process_count = 0;
@@ -64,7 +64,10 @@ struct DataSnapshot {
 
 class DataStore {
 public:
-    // Constructor with dependency injection for platform abstraction
+    // Constructor with dependency injection for platform abstraction.
+    // LIFETIME: The provided pointers must remain valid for the lifetime of
+    // this DataStore. The destructor calls stop() which joins the background
+    // thread, so callers must ensure providers outlive this object.
     DataStore(IProcessDataProvider* process_provider, ISystemDataProvider* system_provider);
     ~DataStore();
 
@@ -104,7 +107,7 @@ class DataStore {
     void collection_thread_func();
     void collect_data();
     static void calculate_tree_totals(ProcessNode& node);
-    static void build_process_map(ProcessNode* node, std::map<int, ProcessNode*>& map);
+    static void build_process_map(ProcessNode* node, std::unordered_map<int, ProcessNode*>& map);
 
     // Injected providers (owned externally)
     IProcessDataProvider* process_provider_;
@@ -131,7 +134,7 @@ class DataStore {
     std::vector<double> per_cpu_usage_buffer_;     // Reused buffer
     std::vector<double> per_cpu_user_buffer_;      // Reused buffer
     std::vector<double> per_cpu_system_buffer_;    // Reused buffer
-    std::map<int, std::pair<uint64_t, uint64_t>> previous_cpu_times_;
+    std::unordered_map<int, std::pair<uint64_t, uint64_t>> previous_cpu_times_;
 
     // Callback
     std::function<void()> on_data_updated_;

src/core/services/single_instance.cpp

@@ -30,10 +30,16 @@ SingleInstance::~SingleInstance() {
 }
 
 std::string SingleInstance::get_socket_path() {
+    constexpr size_t max_path = sizeof(sockaddr_un{}.sun_path) - 1;
+
     if (const char* runtime_dir = std::getenv("XDG_RUNTIME_DIR")) {
-        return std::string(runtime_dir) + "/pex.sock";
+        std::string path = std::string(runtime_dir) + "/pex.sock";
+        if (path.size() <= max_path) {
+            return path;
+        }
+        // XDG_RUNTIME_DIR path too long, fall through to /tmp
     }
-    // Fallback: use /tmp with UID
+    // Fallback: use /tmp with UID (always fits)
     return "/tmp/pex-" + std::to_string(getuid()) + ".sock";
 }
 

src/main.cpp

@@ -21,17 +21,20 @@ int main([[maybe_unused]] int argc, [[maybe_unused]] char* argv[]) {
     }
 
     try {
-        // Create platform-specific providers (owned here in main)
+        // Create platform-specific providers (owned here in main).
+        // IMPORTANT: Declaration order matters for destruction safety.
+        // C++ destroys locals in reverse declaration order, so:
+        //   1. app is destroyed first (stops UI loop)
+        //   2. data_store is destroyed next (joins background thread)
+        //   3. providers are destroyed last (safe, no longer referenced)
+        // Do NOT reorder these declarations without verifying destruction safety.
         const auto process_provider = pex::make_process_data_provider();
         const auto details_provider = pex::make_details_data_provider();
         const auto system_provider = pex::make_system_data_provider();
         const auto killer = pex::make_process_killer();
 
-        // Create DataStore - the data layer that can be shared across UIs
         pex::DataStore data_store(process_provider.get(), system_provider.get());
 
-        // Create and run the ImGui application (UI layer)
-        // ImGuiApp does not own these resources - they're managed here
         pex::ImGuiApp app(&data_store, system_provider.get(), details_provider.get(), killer.get());
 
         instance.set_raise_callback([&app]() {

src/main_tui.cpp

@@ -15,16 +15,20 @@ int main([[maybe_unused]] int argc, [[maybe_unused]] char* argv[]) {
     sigaction(SIGCHLD, &sa, nullptr);
 
     try {
-        // Create platform-specific providers (owned here in main)
+        // Create platform-specific providers (owned here in main).
+        // IMPORTANT: Declaration order matters for destruction safety.
+        // C++ destroys locals in reverse declaration order, so:
+        //   1. app is destroyed first (stops UI loop, restores terminal)
+        //   2. data_store is destroyed next (joins background thread)
+        //   3. providers are destroyed last (safe, no longer referenced)
+        // Do NOT reorder these declarations without verifying destruction safety.
         const auto process_provider = pex::make_process_data_provider();
         const auto details_provider = pex::make_details_data_provider();
         const auto system_provider = pex::make_system_data_provider();
         const auto killer = pex::make_process_killer();
 
-        // Create DataStore - the data layer that can be shared across UIs
         pex::DataStore data_store(process_provider.get(), system_provider.get());
 
-        // Create and run the TUI application (UI layer)
         pex::TuiApp app(&data_store, system_provider.get(), details_provider.get(), killer.get());
         app.run();
         return 0;

src/platform/freebsd/freebsd_process_killer.cpp

@@ -8,11 +8,28 @@
 #include <cstring>
 #include <vector>
 #include <set>
+#include <unordered_map>
 #include <thread>
 #include <chrono>
 
 namespace pex {
 
+namespace {
+
+// Verify a PID still refers to the same process by comparing start times
+bool is_same_process(int pid, const struct timeval& expected_start) {
+    int mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_PID, pid };
+    struct kinfo_proc kp;
+    size_t len = sizeof(kp);
+    if (sysctl(mib, 4, &kp, &len, nullptr, 0) < 0 || len != sizeof(kp)) {
+        return false;
+    }
+    return kp.ki_start.tv_sec == expected_start.tv_sec &&
+           kp.ki_start.tv_usec == expected_start.tv_usec;
+}
+
+} // anonymous namespace
+
 KillResult FreeBSDProcessKiller::kill_process(int pid, bool force) {
     KillResult result;
     if (pid <= 0) {
@@ -90,6 +107,12 @@ KillResult FreeBSDProcessKiller::kill_process_tree(int pid, bool force) {
     size_t count = len / sizeof(struct kinfo_proc);
     struct kinfo_proc* kp = reinterpret_cast<struct kinfo_proc*>(buf.data());
 
+    // Capture start times for PID-reuse verification
+    std::unordered_map<int, struct timeval> start_times;
+    for (size_t i = 0; i < count; ++i) {
+        start_times[kp[i].ki_pid] = kp[i].ki_start;
+    }
+
     // Find all children iteratively
     bool found_new = true;
     while (found_new) {
@@ -106,10 +129,18 @@ KillResult FreeBSDProcessKiller::kill_process_tree(int pid, bool force) {
     int sig = force ? SIGKILL : SIGTERM;
     bool any_success = false;
     bool any_permission_denied = false;
+    int skipped = 0;
 
     // Kill children first (reverse order to kill deepest first)
     std::vector<int> sorted_pids(pids_to_kill.begin(), pids_to_kill.end());
     for (auto it = sorted_pids.rbegin(); it != sorted_pids.rend(); ++it) {
+        // Verify PID hasn't been reused before killing
+        if (auto st = start_times.find(*it); st != start_times.end()) {
+            if (!is_same_process(*it, st->second)) {
+                skipped++;
+                continue;
+            }
+        }
         if (::kill(*it, sig) == 0) {
             any_success = true;
         } else if (errno == EPERM) {

src/platform/linux/linux_process_killer.cpp

@@ -84,12 +84,12 @@ void LinuxProcessKiller::collect_descendants_from_proc(const int root_pid,
                 if (meta.ppid > 0) {
                     children_map[meta.ppid].push_back(pid);
                 }
-            } catch (...) {
+            } catch (const std::exception&) {
                 // Process disappeared, skip it
                 continue;
             }
         }
-    } catch (...) {
+    } catch (const std::exception&) {
         // Directory iteration failed
         return;
     }

src/platform/linux/procfs/procfs_fds.cpp

@@ -46,11 +46,11 @@ std::vector<FileHandleInfo> ProcfsReader::get_file_handles(const int pid) {
                 }
 
                 handles.push_back(std::move(handle));
-            } catch (...) {
+            } catch (const std::exception&) {
                 continue;
             }
         }
-    } catch (...) {
+    } catch (const std::exception&) {
     }
 
     std::ranges::sort(handles, [](const auto& a, const auto& b) {

src/platform/linux/procfs/procfs_network.cpp

@@ -116,11 +116,11 @@ std::vector<NetworkConnectionInfo> ProcfsReader::get_network_connections(const i
                     std::from_chars(start, end, inode);
                     if (inode > 0) socket_inodes.insert(inode);
                 }
-            } catch (...) {
+            } catch (const std::exception&) {
                 continue;
             }
         }
-    } catch (...) {
+    } catch (const std::exception&) {
         return result;
     }
 

src/platform/linux/procfs/procfs_processes.cpp

@@ -34,7 +34,7 @@ std::vector<ProcessInfo> ProcfsReader::get_all_processes(const int64_t total_mem
                 if (auto info = get_process_info(pid, total_memory)) {
                     processes.push_back(std::move(*info));
                 }
-            } catch (...) {
+            } catch (const std::exception&) {
                 continue;
             }
         }

src/platform/linux/procfs/procfs_threads.cpp

@@ -57,9 +57,13 @@ std::vector<ThreadInfo> ProcfsReader::get_threads(int pid) {
     }
 
     auto find_library = [&](const uint64_t addr) -> std::string {
-        for (const auto& range : address_map) {
-            if (addr >= range.start && addr < range.end) {
-                return range.library;
+        // address_map is sorted by start address (from /proc/pid/maps)
+        auto it = std::upper_bound(address_map.begin(), address_map.end(), addr,
+            [](uint64_t a, const AddressRange& r) { return a < r.start; });
+        if (it != address_map.begin()) {
+            --it;
+            if (addr >= it->start && addr < it->end) {
+                return it->library;
             }
         }
         return {};
@@ -145,11 +149,11 @@ std::vector<ThreadInfo> ProcfsReader::get_threads(int pid) {
                 }
 
                 threads.push_back(std::move(thread));
-            } catch (...) {
+            } catch (const std::exception&) {
                 continue;
             }
         }
-    } catch (...) {
+    } catch (const std::exception&) {
     }
 
     return threads;

src/platform/solaris/solaris_process_data_provider.cpp

@@ -122,7 +122,7 @@ std::optional<ProcessInfo> SolarisProcessDataProvider::read_process_info(int pid
     std::string exe_path = proc_path + "/path/a.out";
     try {
         info.executable_path = fs::read_symlink(exe_path).string();
-    } catch (...) {
+    } catch (const std::exception&) {
         info.executable_path = info.name;
     }
 
@@ -145,7 +145,7 @@ std::vector<ProcessInfo> SolarisProcessDataProvider::get_all_processes(int64_t t
             int pid = 0;
             try {
                 pid = std::stoi(name);
-            } catch (...) {
+            } catch (const std::exception&) {
                 continue;  // Not a PID directory
             }