From e6258786782604eab2db478a8fccb40ccfd475e4 Mon Sep 17 00:00:00 2001 From: r1viollet Date: Fri, 19 Jan 2024 14:36:36 +0100 Subject: [PATCH] Add a limit on maximum number of profiled PIDs --- include/common_symbol_errors.hpp | 1 + include/ddprof_defs.hpp | 4 ++++ include/ddres_list.hpp | 1 + include/dwfl_hdr.hpp | 3 ++- src/common_symbol_lookup.cc | 2 ++ src/ddprof_worker.cc | 2 +- src/dwfl_hdr.cc | 9 ++++++--- src/unwind.cc | 21 +++++++++++---------- src/unwind_dwfl.cc | 7 +++++-- 9 files changed, 33 insertions(+), 17 deletions(-) diff --git a/include/common_symbol_errors.hpp b/include/common_symbol_errors.hpp index ce42955e0..036515ce4 100644 --- a/include/common_symbol_errors.hpp +++ b/include/common_symbol_errors.hpp @@ -12,6 +12,7 @@ enum SymbolErrors { unknown_dso, dwfl_frame, incomplete_stack, + max_pids, lost_event, }; diff --git a/include/ddprof_defs.hpp b/include/ddprof_defs.hpp index 0fe723b97..a07eafb60 100644 --- a/include/ddprof_defs.hpp +++ b/include/ddprof_defs.hpp @@ -31,6 +31,10 @@ inline constexpr auto k_min_number_samples_per_ring_buffer = 8; inline constexpr int k_size_api_key = 32; +// Maximum number of profiled pids +// Exceeding a number of PIDs overloads file descriptors and memory +inline constexpr unsigned kMaxProfiledPids{100}; + // Linux Inode type using inode_t = uint64_t; diff --git a/include/ddres_list.hpp b/include/ddres_list.hpp index d2f512e73..84f147113 100644 --- a/include/ddres_list.hpp +++ b/include/ddres_list.hpp @@ -23,6 +23,7 @@ enum { DD_COMMON_START_RANGE = 1000, DD_NATIVE_START_RANGE = 2000 }; X(DWFL_LIB_ERROR, "error withing dwfl library") \ X(UW_CACHE_ERROR, "error from unwinding cache") \ X(UW_ERROR, "error from unwinding code") \ + X(UW_MAX_PIDS, "Maximum number of PIDs reached") \ X(UW_MAX_DEPTH, "max depth reached in unwinding") \ X(CAPLIB, "error when reading capabilities") \ X(USERID, "error in user ID manipulations") \ diff --git a/include/dwfl_hdr.hpp b/include/dwfl_hdr.hpp index 281670722..b0c5beea0 100644 --- a/include/dwfl_hdr.hpp +++ b/include/dwfl_hdr.hpp @@ -61,7 +61,8 @@ struct DwflWrapper { class DwflHdr { public: - DwflWrapper &get_or_insert(pid_t pid); + // checks against maximum number of PIDs + DwflWrapper *get_or_insert(pid_t pid); std::vector get_unvisited() const; void reset_unvisited(); void clear_pid(pid_t pid); diff --git a/src/common_symbol_lookup.cc b/src/common_symbol_lookup.cc index 5399997ea..d898dfc94 100644 --- a/src/common_symbol_lookup.cc +++ b/src/common_symbol_lookup.cc @@ -19,6 +19,8 @@ Symbol symbol_from_common(SymbolErrors lookup_case) { return {std::string(), std::string("[incomplete]"), 0, std::string()}; case SymbolErrors::lost_event: return {std::string(), std::string("[lost]"), 0, std::string()}; + case SymbolErrors::max_pids: + return {std::string(), std::string("[maximum pids]"), 0, std::string()}; default: break; } diff --git a/src/ddprof_worker.cc b/src/ddprof_worker.cc index 82994f7a3..0f29ba345 100644 --- a/src/ddprof_worker.cc +++ b/src/ddprof_worker.cc @@ -228,7 +228,7 @@ DDRes ddprof_unwind_sample(DDProfContext &ctx, perf_event_sample *sample, ddprof_stats_add(STATS_UNWIND_TRUNCATED_INPUT, 1, nullptr); } - if (us->_dwfl_wrapper->_inconsistent) { + if (us->_dwfl_wrapper && us->_dwfl_wrapper->_inconsistent) { // Loaded modules were inconsistent, assume we should flush everything. LG_WRN("(Inconsistent DWFL/DSOs)%d - Free associated objects", us->pid); DDRES_CHECK_FWD(worker_pid_free(ctx, us->pid)); diff --git a/src/dwfl_hdr.cc b/src/dwfl_hdr.cc index c68ed7485..b7daa10dd 100644 --- a/src/dwfl_hdr.cc +++ b/src/dwfl_hdr.cc @@ -47,16 +47,19 @@ DDRes DwflWrapper::attach(pid_t pid, const Dwfl_Thread_Callbacks *callbacks, return {}; } -DwflWrapper &DwflHdr::get_or_insert(pid_t pid) { +DwflWrapper *DwflHdr::get_or_insert(pid_t pid) { _visited_pid.insert(pid); auto it = _dwfl_map.find(pid); if (it == _dwfl_map.end()) { + if (_dwfl_map.size() >= kMaxProfiledPids) { + return nullptr; + } // insert new dwfl for this pid auto pair = _dwfl_map.emplace(pid, DwflWrapper()); assert(pair.second); // expect insertion to be OK - return pair.first->second; + return &pair.first->second; } - return it->second; + return &it->second; } DDProfMod *DwflWrapper::unsafe_get(FileInfoId_t file_info_id) { diff --git a/src/unwind.cc b/src/unwind.cc index 023ca0f3c..9899665c2 100644 --- a/src/unwind.cc +++ b/src/unwind.cc @@ -64,11 +64,15 @@ bool is_stack_complete(UnwindState *us) { root_func) != s_expected_root_frames.end(); } -void find_dso_add_error_frame(UnwindState *us) { - DsoHdr::DsoFindRes const find_res = - us->dso_hdr.dso_find_closest(us->pid, us->current_ip); - add_error_frame(find_res.second ? &(find_res.first->second) : nullptr, us, - us->current_ip); +void find_dso_add_error_frame(DDRes ddres, UnwindState *us) { + if (ddres._what == DD_WHAT_UW_MAX_PIDS) { + add_common_frame(us, SymbolErrors::max_pids); + } else { + DsoHdr::DsoFindRes const find_res = + us->dso_hdr.dso_find_closest(us->pid, us->current_ip); + add_error_frame(find_res.second ? &(find_res.first->second) : nullptr, us, + us->current_ip); + } } void add_container_id(UnwindState *us) { @@ -99,17 +103,14 @@ DDRes unwindstate_unwind(UnwindState *us) { res = unwind_dwfl(us); } if (IsDDResNotOK(res)) { - find_dso_add_error_frame(us); - } - - if (!is_stack_complete(us)) { + find_dso_add_error_frame(res, us); + } else if (!is_stack_complete(us)) { us->output.is_incomplete = true; ddprof_stats_add(STATS_UNWIND_INCOMPLETE_STACK, 1, nullptr); // Only add [incomplete] virtual frame if stack is not already truncated ! if (!is_max_stack_depth_reached(*us)) { add_common_frame(us, SymbolErrors::incomplete_stack); } - } else { us->output.is_incomplete = false; } diff --git a/src/unwind_dwfl.cc b/src/unwind_dwfl.cc index c533400ab..e08716fe0 100644 --- a/src/unwind_dwfl.cc +++ b/src/unwind_dwfl.cc @@ -252,7 +252,10 @@ DDRes add_runtime_symbol_frame(UnwindState *us, const Dso &dso, ElfAddress_t pc, DDRes unwind_init_dwfl(UnwindState *us) { // Create or get the dwfl object associated to cache - us->_dwfl_wrapper = &(us->dwfl_hdr.get_or_insert(us->pid)); + us->_dwfl_wrapper = us->dwfl_hdr.get_or_insert(us->pid); + if (!us->_dwfl_wrapper) { + return ddres_warn(DD_WHAT_UW_MAX_PIDS); + } if (!us->_dwfl_wrapper->_attached) { // we need to add at least one module to figure out the architecture (to // create the unwinding backend) @@ -267,7 +270,7 @@ DDRes unwind_init_dwfl(UnwindState *us) { // Find an elf file we can load for this PID for (auto it = map.cbegin(); it != map.cend(); ++it) { const Dso &dso = it->second; - if (dso.is_executable()) { + if (dso.is_executable() && dso._type == DsoType::kStandard) { FileInfoId_t const file_info_id = us->dso_hdr.get_or_insert_file_info(dso); if (file_info_id <= k_file_info_error) {