llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Raphael Isemann (Teemperor) <details> <summary>Changes</summary> cbd76dcfa81e58dbaca0 fixes a race condition when fetching process lists using sysctl. However, the same bug also exists also in the *BSD host libraries and in debugserver. This patch merges the process list fetching logic of Darwin and the *BSD plugins and uses the race-free fetching approach. I moved the shared logic into the Host/posix/ folder even though the sysctl call is not part of the POSIX standard. However, we don't have a better fitting existing folder that covers UNIX-like systems. --- Full diff: https://github.com/llvm/llvm-project/pull/205768.diff 6 Files Affected: - (added) lldb/include/lldb/Host/posix/GetProcessList.h (+26) - (modified) lldb/source/Host/CMakeLists.txt (+3) - (modified) lldb/source/Host/freebsd/Host.cpp (+7-19) - (modified) lldb/source/Host/macosx/objcxx/Host.mm (+2-69) - (modified) lldb/source/Host/openbsd/Host.cpp (+7-18) - (added) lldb/source/Host/posix/GetProcessList.cpp (+89) ``````````diff diff --git a/lldb/include/lldb/Host/posix/GetProcessList.h b/lldb/include/lldb/Host/posix/GetProcessList.h new file mode 100644 index 0000000000000..35b47d125f214 --- /dev/null +++ b/lldb/include/lldb/Host/posix/GetProcessList.h @@ -0,0 +1,26 @@ +//===-- GetProcessList.h ----------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_HOST_POSIX_GETPROCESSLIST_H +#define LLDB_HOST_POSIX_GETPROCESSLIST_H + +#include "llvm/Support/Error.h" + +#include <sys/sysctl.h> +#include <vector> + +namespace lldb_private { + +/// Fetches the list of all processes into \p kinfos using the BSD/Darwin +/// sysctl(KERN_PROC_ALL) interface. Note that this is not technically a POSIX +/// interface but used in several UNIX-like systems. +llvm::Error GetProcessList(std::vector<struct kinfo_proc> &kinfos); + +} // namespace lldb_private + +#endif // LLDB_HOST_POSIX_GETPROCESSLIST_H diff --git a/lldb/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt index 28c639ee38215..11fc5b1c9ba04 100644 --- a/lldb/source/Host/CMakeLists.txt +++ b/lldb/source/Host/CMakeLists.txt @@ -123,6 +123,7 @@ else() macosx/cfcpp/CFCMutableDictionary.cpp macosx/cfcpp/CFCMutableSet.cpp macosx/cfcpp/CFCString.cpp + posix/GetProcessList.cpp ) if (LLDB_ENABLE_PYTHON) add_host_subdirectory(macosx @@ -157,6 +158,7 @@ else() add_host_subdirectory(freebsd freebsd/Host.cpp freebsd/HostInfoFreeBSD.cpp + posix/GetProcessList.cpp ) elseif (CMAKE_SYSTEM_NAME MATCHES "NetBSD") @@ -169,6 +171,7 @@ else() add_host_subdirectory(openbsd openbsd/Host.cpp openbsd/HostInfoOpenBSD.cpp + posix/GetProcessList.cpp ) elseif (CMAKE_SYSTEM_NAME MATCHES "AIX") diff --git a/lldb/source/Host/freebsd/Host.cpp b/lldb/source/Host/freebsd/Host.cpp index dfdbfea0c3c0a..dada5c19653b2 100644 --- a/lldb/source/Host/freebsd/Host.cpp +++ b/lldb/source/Host/freebsd/Host.cpp @@ -21,9 +21,11 @@ #include "lldb/Host/FileSystem.h" #include "lldb/Host/Host.h" #include "lldb/Host/HostInfo.h" +#include "lldb/Host/posix/GetProcessList.h" #include "lldb/Utility/DataBufferHeap.h" #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/Endian.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/NameMatches.h" #include "lldb/Utility/ProcessInfo.h" @@ -170,30 +172,16 @@ uint32_t Host::FindProcessesImpl(const ProcessInstanceInfoMatch &match_info, // Special case, if lldb is being run as root we can attach to anything. bool all_users = match_info.GetMatchAllUsers() || (our_uid == 0); - int mib[3] = {CTL_KERN, KERN_PROC, KERN_PROC_ALL}; - - size_t pid_data_size = 0; - if (::sysctl(mib, 3, NULL, &pid_data_size, NULL, 0) != 0) - return 0; - - // Add a few extra in case a few more show up - const size_t estimated_pid_count = - (pid_data_size / sizeof(struct kinfo_proc)) + 10; - - kinfos.resize(estimated_pid_count); - pid_data_size = kinfos.size() * sizeof(struct kinfo_proc); - - if (::sysctl(mib, 3, &kinfos[0], &pid_data_size, NULL, 0) != 0) + if (llvm::Error error = GetProcessList(kinfos)) { + LLDB_LOG_ERROR(GetLog(LLDBLog::Host | LLDBLog::Process), std::move(error), + "failed to read process list: {0}"); return 0; - - const size_t actual_pid_count = (pid_data_size / sizeof(struct kinfo_proc)); + } ProcessInstanceInfoMatch match_info_noname{match_info}; match_info_noname.SetNameMatchType(NameMatch::Ignore); - for (size_t i = 0; i < actual_pid_count; i++) { - const struct kinfo_proc &kinfo = kinfos[i]; - + for (const struct kinfo_proc &kinfo : kinfos) { /* Make sure the user is acceptable */ if (!all_users && kinfo.ki_ruid != our_uid) continue; diff --git a/lldb/source/Host/macosx/objcxx/Host.mm b/lldb/source/Host/macosx/objcxx/Host.mm index 13ee1cefb6cea..05888c6c7e389 100644 --- a/lldb/source/Host/macosx/objcxx/Host.mm +++ b/lldb/source/Host/macosx/objcxx/Host.mm @@ -60,12 +60,13 @@ #include "lldb/Host/HostInfo.h" #include "lldb/Host/ProcessLaunchInfo.h" #include "lldb/Host/ThreadLauncher.h" +#include "lldb/Host/posix/GetProcessList.h" #include "lldb/Utility/ArchSpec.h" -#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/DataBufferHeap.h" #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/Endian.h" #include "lldb/Utility/FileSpec.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/NameMatches.h" #include "lldb/Utility/ProcessInfo.h" @@ -688,74 +689,6 @@ static bool GetMacOSXProcessUserAndGroup(ProcessInstanceInfo &process_info) { return false; } -/// Fetches the list of all processes into \p kinfos. -static llvm::Error GetProcessList(std::vector<struct kinfo_proc> &kinfos) { - int mib[3] = {CTL_KERN, KERN_PROC, KERN_PROC_ALL}; - - // How often we retry fetching the process list. - static constexpr unsigned g_retry_count = 200; - // The rate at which we increase the adjusted buffer size to - // account for newly created processes between two sysctl calls. - static constexpr unsigned g_expected_new_pids = 500; - // We keep increasing the expected growth rate between the two - // sysctl calls. Check that the last attempt does not create an - // unreasonbly large buffer. It is unlikely we run on a system where - // 100k processes are repeatedly created between each attempted sysctl - // pair. - static_assert( - g_retry_count * g_expected_new_pids <= 100'000, - "Final retry attempt assumes an unlikely amount of new processes."); - - // This is an inherently racy API. We have to first query the size for our - // buffer and then pass it back to sysctl. If more processes spawn between the - // size query and the actual call to fetch, then sysctl returns ENOMEM. - // We keep retrying until we get a passing result. - for (unsigned attempt = 1; attempt < g_retry_count; ++attempt) { - // Fetch the buffer size sysctl would return. - size_t current_pid_size = 0; - if (::sysctl(mib, 3, nullptr, ¤t_pid_size, nullptr, 0) != 0) - return llvm::errorCodeToError( - std::error_code(errno, std::generic_category())); - - // Convert the byte length result to number of elements. - const size_t current_num_processes = - current_pid_size / sizeof(struct kinfo_proc); - - // Adjust the buffer for new processes that spawned between the - // previous and next sysctl call. We increase this growth each attempt - // to account for systems where a lot of new processes spawn between - // these two calls. - const size_t expected_growth = attempt * g_expected_new_pids; - - // Allocate the buffer for the sysctl result. - kinfos.resize(current_num_processes + expected_growth); - - // Fetch the actual process list and let sysctl adjust actual_pid_size. - size_t actual_pid_size = kinfos.size() * sizeof(struct kinfo_proc); - if (::sysctl(mib, 3, &kinfos[0], &actual_pid_size, nullptr, 0) == 0) { - // Shrink the buffer to the actual number of processes returned. - kinfos.resize(actual_pid_size / sizeof(struct kinfo_proc)); - return llvm::Error::success(); - } - - // Errno is set to ENOMEM if our estimated_pid_size is too small, in which - // case we retry with a bigger buffer. Any other error is unexpected. - if (errno != ENOMEM) - return llvm::errorCodeToError( - std::error_code(errno, std::generic_category())); - } - - // The only way to exit the loop above is by repeatedly hitting ENOMEM. The - // only way this can happen is if the process list somehow grew extremely - // large between the two sysctl calls. - assert(errno == ENOMEM && - "loop should only be left via the ENOMEM retry path"); - return llvm::createStringErrorV( - "Failed to read process list: sysctl kept returning ENOMEM after {0} " - "attempts", - g_retry_count); -} - uint32_t Host::FindProcessesImpl(const ProcessInstanceInfoMatch &match_info, ProcessInstanceInfoList &process_infos) { std::vector<struct kinfo_proc> kinfos; diff --git a/lldb/source/Host/openbsd/Host.cpp b/lldb/source/Host/openbsd/Host.cpp index 2b66a3c8696b1..3bd68bd20fb6d 100644 --- a/lldb/source/Host/openbsd/Host.cpp +++ b/lldb/source/Host/openbsd/Host.cpp @@ -19,9 +19,11 @@ #include "lldb/Host/Host.h" #include "lldb/Host/HostInfo.h" +#include "lldb/Host/posix/GetProcessList.h" #include "lldb/Utility/DataBufferHeap.h" #include "lldb/Utility/DataExtractor.h" #include "lldb/Utility/Endian.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/NameMatches.h" #include "lldb/Utility/ProcessInfo.h" @@ -127,29 +129,16 @@ uint32_t Host::FindProcessesImpl(const ProcessInstanceInfoMatch &match_info, ProcessInstanceInfoList &process_infos) { std::vector<struct kinfo_proc> kinfos; - int mib[3] = {CTL_KERN, KERN_PROC, KERN_PROC_ALL}; - - size_t pid_data_size = 0; - if (::sysctl(mib, 3, NULL, &pid_data_size, NULL, 0) != 0) + if (llvm::Error error = GetProcessList(kinfos)) { + LLDB_LOG_ERROR(GetLog(LLDBLog::Host | LLDBLog::Process), std::move(error), + "failed to read process list: {0}"); return 0; - - // Add a few extra in case a few more show up - const size_t estimated_pid_count = - (pid_data_size / sizeof(struct kinfo_proc)) + 10; - - kinfos.resize(estimated_pid_count); - pid_data_size = kinfos.size() * sizeof(struct kinfo_proc); - - if (::sysctl(mib, 3, &kinfos[0], &pid_data_size, NULL, 0) != 0) - return 0; - - const size_t actual_pid_count = (pid_data_size / sizeof(struct kinfo_proc)); + } bool all_users = match_info.GetMatchAllUsers(); const ::pid_t our_pid = getpid(); const uid_t our_uid = getuid(); - for (size_t i = 0; i < actual_pid_count; i++) { - const struct kinfo_proc &kinfo = kinfos[i]; + for (const struct kinfo_proc &kinfo : kinfos) { const bool kinfo_user_matches = (all_users || (kinfo.p_ruid == our_uid) || // Special case, if lldb is being run as // root we can attach to anything. diff --git a/lldb/source/Host/posix/GetProcessList.cpp b/lldb/source/Host/posix/GetProcessList.cpp new file mode 100644 index 0000000000000..e6ce2814ff95c --- /dev/null +++ b/lldb/source/Host/posix/GetProcessList.cpp @@ -0,0 +1,89 @@ +//===-- GetProcessList.cpp ------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Host/posix/GetProcessList.h" + +#include "llvm/Support/Error.h" +#include "llvm/Support/ErrorExtras.h" + +#include <cassert> +#include <cerrno> +#include <cstddef> +#include <sys/sysctl.h> +#include <sys/types.h> +// struct kinfo_proc lives in <sys/user.h> on some BSDs (e.g. FreeBSD). +#include <sys/user.h> +#include <system_error> + +llvm::Error +lldb_private::GetProcessList(std::vector<struct kinfo_proc> &kinfos) { + int mib[3] = {CTL_KERN, KERN_PROC, KERN_PROC_ALL}; + + // How often we retry fetching the process list. + static constexpr unsigned g_retry_count = 200; + // The rate at which we increase the adjusted buffer size to + // account for newly created processes between two sysctl calls. + static constexpr unsigned g_expected_new_pids = 500; + // We keep increasing the expected growth rate between the two + // sysctl calls. Check that the last attempt does not create an + // unreasonbly large buffer. It is unlikely we run on a system where + // 100k processes are repeatedly created between each attempted sysctl + // pair. + static_assert( + g_retry_count * g_expected_new_pids <= 100'000, + "Final retry attempt assumes an unlikely amount of new processes."); + + // This is an inherently racy API. We have to first query the size for our + // buffer and then pass it back to sysctl. If more processes spawn between the + // size query and the actual call to fetch, then sysctl returns ENOMEM. + // We keep retrying until we get a passing result. + for (unsigned attempt = 1; attempt < g_retry_count; ++attempt) { + // Fetch the buffer size sysctl would return. + size_t current_pid_size = 0; + if (::sysctl(mib, 3, nullptr, ¤t_pid_size, nullptr, 0) != 0) + return llvm::errorCodeToError( + std::error_code(errno, std::generic_category())); + + // Convert the byte length result to number of elements. + const size_t current_num_processes = + current_pid_size / sizeof(struct kinfo_proc); + + // Adjust the buffer for new processes that spawned between the + // previous and next sysctl call. We increase this growth each attempt + // to account for systems where a lot of new processes spawn between + // these two calls. + const size_t expected_growth = attempt * g_expected_new_pids; + + // Allocate the buffer for the sysctl result. + kinfos.resize(current_num_processes + expected_growth); + + // Fetch the actual process list and let sysctl adjust actual_pid_size. + size_t actual_pid_size = kinfos.size() * sizeof(struct kinfo_proc); + if (::sysctl(mib, 3, &kinfos[0], &actual_pid_size, nullptr, 0) == 0) { + // Shrink the buffer to the actual number of processes returned. + kinfos.resize(actual_pid_size / sizeof(struct kinfo_proc)); + return llvm::Error::success(); + } + + // Errno is set to ENOMEM if our estimated_pid_size is too small, in which + // case we retry with a bigger buffer. Any other error is unexpected. + if (errno != ENOMEM) + return llvm::errorCodeToError( + std::error_code(errno, std::generic_category())); + } + + // The only way to exit the loop above is by repeatedly hitting ENOMEM. The + // only way this can happen is if the process list somehow grew extremely + // large between the two sysctl calls. + assert(errno == ENOMEM && + "loop should only be left via the ENOMEM retry path"); + return llvm::createStringErrorV( + "Failed to read process list: sysctl kept returning ENOMEM after {0} " + "attempts", + g_retry_count); +} `````````` </details> https://github.com/llvm/llvm-project/pull/205768 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
