Rocprofsys attach#1
Conversation
WalkthroughAdds runtime attach/detach support. Introduces a new attach library with ptrace-based injection, environment propagation, and SIGUSR1-driven detach. Extends sample binary with attach CLI and flow. Updates core config/state, signal handling, and library lifecycle to support pre-attach mode and re-attach. Adds build integration and minor idempotency guards. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sample as rocprof-sys-sample
participant AttachLib as rocprof-sys-attach.so
participant Target as Target Process
participant DL as librocprof-sys-dl.so
User->>Sample: run with --attach-pid/-p [--attach-duration-msec]
Sample->>AttachLib: rocprofsys_attach(pid, env)
AttachLib->>Target: ptrace attach/stop
AttachLib->>Target: mmap + write env buffer
AttachLib->>Target: dlopen(librocprof-sys-dl.so)
AttachLib->>DL: call rocprofsys_dl_attach(env_buf_addr)
DL->>DL: load env, set ROCPROFSYS_ATTACH_PID
DL->>Target: rocprofsys_init (attach mode) + enable tracing
AttachLib->>Target: ptrace detach/continue
alt duration > 0
Sample-->>Sample: sleep(duration)
else
Sample-->>User: "Press any key to detach"
User->>Sample: keypress
end
Sample->>AttachLib: rocprofsys_detach(pid)
AttachLib->>Target: send SIGUSR1 (detach)
Note right of Target: SIGUSR1 handled by config<br/>triggers detach_handler
Target->>AttachLib: write byte to FIFO (/tmp/rocprofsys_detach_pipe)
AttachLib-->>Sample: return
sequenceDiagram
autonumber
participant Kernel as Target (in-process)
participant Config as core::config
participant Lib as rocprof-sys library
participant FIFO as /tmp/rocprofsys_detach_pipe
Kernel->>Config: SIGUSR1 handler invoked
Config->>Lib: detach_handler()
alt attach mode and Active
Lib->>Lib: finalize tooling (attach-safe)
Lib->>FIFO: write 1 byte (detach complete)
Lib->>Lib: set State=Detached
else not in pre-attach
Lib-->>Lib: no-op (message)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp (1)
1876-1878: Call stop() before flush() to reduce lost data.Flushing buffers before stopping contexts risks
ERROR_BUFFER_BUSYand dropped records. Swap the order in tool_fini.Apply:
- flush(); - stop(); + stop(); + flush();projects/rocprofiler-systems/source/lib/rocprof-sys/library/sampling.cpp (2)
594-605: Use tellp() when writing; tellg() is for reads.Writing uses
_fs.write(...)but records positions via_fs.tellg(). This can return invalid positions or -1. Usetellp().- offload_seq_data[_seq].emplace(_fs.tellg()); + offload_seq_data[_seq].emplace(_fs.tellp());
1179-1196: Remove offload tmp file before resetting the shared_ptr; current order skips removal.
get_offload_file().reset();nulls the handle, so the subsequentif(get_offload_file())never runs.- get_offload_file().reset(); // remove the temporary file + if(get_use_tmp_files()) + { + auto& _f = get_offload_file(); + if(_f) _f->remove(); + _f.reset(); + } - - if(get_use_tmp_files() && get_offload_file()) - { - get_offload_file()->remove(); - get_offload_file().reset(); - }
🧹 Nitpick comments (22)
projects/rocprofiler-systems/source/lib/core/state.cpp (1)
137-137: String mapping for Detached added—LGTM.Covers the new enum; consistent with logging needs.
Consider keeping case order consistent with the enum for readability.
projects/rocprofiler-systems/source/lib/core/config.hpp (1)
51-52: Detach signal handler API addition—LGTM.Symmetric with set_signal_handler; ensure docs mention interaction with existing handler and lifetime.
projects/rocprofiler-systems/source/lib/core/config.cpp (2)
251-252: Attach mode is derived only from ROCPROFSYS_ATTACH_PID; ignores new ROCPROFSYS_ATTACHYou add a ROCPROFSYS_ATTACH setting below, but
is_attach_mode()is set solely fromROCPROFSYS_ATTACH_PID. Either honor the boolean setting here or recompute after settings load to keep behavior coherent.Example tweak (early env + setting):
- is_attach_mode() = tim::get_env("ROCPROFSYS_ATTACH_PID", 0) != 0; + // pre-attach if pid provided or explicit boolean flag set + const bool attach_env = tim::get_env("ROCPROFSYS_ATTACH", false, false); + is_attach_mode() = attach_env || tim::get_env("ROCPROFSYS_ATTACH_PID", 0) != 0;And/or after
_configis available:+ // re-sync after settings load + is_attach_mode() = is_attach_mode() || _config->get<bool>("ROCPROFSYS_ATTACH");
264-267: New setting ROCPROFSYS_ATTACH is never readYou define the setting but never consume it to influence attach behavior. Wire it into
is_attach_mode()per the previous comment.projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp (1)
907-921: Comment vs behavior mismatch: “Stop instead of shutting down” but you call shutdownIn attach mode you execute both
stop()andshutdown(). Either:
- Only stop (keep SDK client intact for re‑attach), or
- Keep shutdown but update the comment.
If re‑attach requires full re-init anyway, prefer being explicit and skipping
stop(); otherwise removeshutdown()here.- if(_is_attach) - { - ROCPROFSYS_VERBOSE_F(1, "Shutting down ROCm in attach mode...\n"); - rocprofiler_sdk::stop(); - rocprofiler_sdk::shutdown(); - } + if(_is_attach) + { + ROCPROFSYS_VERBOSE_F(1, "Stopping ROCm contexts for detach...\n"); + rocprofiler_sdk::stop(); + // Skip shutdown in attach mode to enable faster re-attach + }projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.hpp (6)
1-11: Avoid heavy/OS‑specific includes in public headers
<dlfcn.h>is not used in the declarations; move it to the .cpp to reduce header surface.-#include <dlfcn.h>
17-24: Type choices: prefer pid_t for PIDs and uintptr_t for addressesUsing
size_tfor PIDs/addresses may break on cross‑arch or 32/64‑bit assumptions. Consider:
- ctor:
explicit PTraceSession(pid_t);- address params:
uintptr_t(orstd::uintptr_t) instead ofsize_t.- explicit PTraceSession(size_t); + explicit PTraceSession(pid_t);And for memory APIs:
- bool write(size_t addr, const std::vector<uint8_t>& data, size_t size); + bool write(std::uintptr_t addr, const std::vector<uint8_t>& data, size_t size);Apply similarly to
read/swap/call_functionoverloads andget_pid().
35-47: Open‑ended return type for open_library
unsigned long longfor handles is ambiguous. If this is an address, preferstd::uintptr_t; if a status code, use a strong typedef or enum.
58-60: Member types alignment with earlier note
const size_t pid;should align withpid_t.- const size_t pid; + const pid_t pid;
64-72: NullBuffer overflow signatureUse the standard
int_typesignature to avoid surprises across libstdc++/libc++.-class NullBuffer : public std::streambuf -{ -public: - int overflow(int c) override { return c; } -}; +class NullBuffer : public std::streambuf { +public: + using int_type = std::streambuf::traits_type::int_type; + int_type overflow(int_type c) override { return c; } +};
73-80: Header‑level logging macros can leak into consumersDefining
ROCPROFSYS_INFO/ERRORin a public header risks macro collisions. Prefer placing them in a private header for the attach module, or undef after use.#ifndef ROCPROFSYS_INFO # define ROCPROFSYS_INFO \ ((rocprofsys::attach::get_verbose() > 1) ? std::cout : rocprofsys_null_stream) #endifprojects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.cpp (1)
58-62: Propagate attach() failure and fix typo in comment.Return the status from
attach()and correct “greate”.- if(*(get_pid()) > 0) + if(*(get_pid()) > 0) { - // If the pid is greate than 0, it means we are running in the attach mode. - return attach(_env); + // If the pid is greater than 0, it means we are running in attach mode. + int rc = attach(_env); + return rc; }projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.cpp (2)
37-43: Remove dead code (get_dl_handle).Unused helper can be dropped.
-void** -get_dl_handle() -{ - static void* handle = nullptr; - return &handle; -}
59-63: Prefer pid_t over size_t for pid parameters here and in headers.Matches POSIX APIs (
kill,ptrace). Update declarations if feasible.projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp (1)
38-47: Optional: avoid blocking on stdin when not a TTY.If run non-interactively,
std::cin.get()blocks forever. Consider checkingisatty(STDIN_FILENO)and falling back to a default timeout.projects/rocprofiler-systems/source/lib/rocprof-sys-attach/CMakeLists.txt (3)
23-23: Do not set CMAKE_BUILD_TYPE in a subdirectory.This overrides parent/multi-config generators. Remove this line and let the top-level or the generator control configuration.
-set(CMAKE_BUILD_TYPE "Release")
60-61: Scope debug flags to the target (and typically not PUBLIC).-g3 on a library that’s also stripped is contradictory and leaking debug flags to dependents is undesirable. Make it PRIVATE or gate by build type.
-add_target_cxx_flag_if_avail(rocprofiler-systems-attach-library "-g3") +# Prefer a scoped compile option: +target_compile_options(rocprofiler-systems-attach-library PRIVATE -g3)
69-71: Library RPATH set to $ORIGIN.Setting RPATH on a shared library can have side effects in downstream consumers. If you only need $ORIGIN for executables or tests, consider confining it there.
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.cpp (4)
174-182: Use WUNTRACED with waitpid and check statuses.WUNTRACED is the canonical flag; also verify WIFSTOPPED/WSTOPSIG after each waitpid.
- if(waitpid(pid, 0, WSTOPPED) == -1) + if(waitpid(pid, 0, WUNTRACED) == -1) return false;Also applies to: 262-270, 582-590
187-201: PAGE_SIZE is not defined here.Use sysconf(_SC_PAGESIZE) or getpagesize() to compute length when 0.
- newregs.rsi = (length == 0) ? PAGE_SIZE : length; // length + const size_t page_sz = static_cast<size_t>(sysconf(_SC_PAGESIZE)); + newregs.rsi = (length == 0) ? page_sz : length; // length
436-470: find_library(): fragile match and error handling.
- Substring match on "library" may miss SONAME variants (e.g., libc-2.39.so). Consider matching by basename or regex.
- If no line found, current logic may still attempt stoull (guarded by maps state, but be explicit).
13-22: Inconsistent logging macros; potential build break.File defines ROCPROFSYS_ATTACH_LOG but uses ROCPROFSYS_INFO/ERROR streams. Ensure these are defined via a shared logging header, or add local fallbacks. Also fix printf format specifiers in logging (size_t → %zu).
Would you like a patch to introduce a lightweight fallback stream logger gated by get_verbose()?
Also applies to: 55-61, 457-459, 526-540, 544-552, 556-571
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/CMakeLists.txt(1 hunks)projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp(5 hunks)projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.cpp(2 hunks)projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.hpp(2 hunks)projects/rocprofiler-systems/source/lib/CMakeLists.txt(1 hunks)projects/rocprofiler-systems/source/lib/core/config.cpp(7 hunks)projects/rocprofiler-systems/source/lib/core/config.hpp(2 hunks)projects/rocprofiler-systems/source/lib/core/perfetto.cpp(1 hunks)projects/rocprofiler-systems/source/lib/core/state.cpp(1 hunks)projects/rocprofiler-systems/source/lib/core/state.hpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/CMakeLists.txt(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.cpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.hpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.cpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.hpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys-dl/dl.cpp(3 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp(10 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.hpp(1 hunks)projects/rocprofiler-systems/source/lib/rocprof-sys/library/sampling.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.cpp (1)
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.cpp (4)
get_verbose(605-610)get_verbose(606-606)read(122-152)read(123-123)
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.cpp (1)
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp (4)
get_pid(905-910)get_pid(906-906)attach(919-939)attach(920-920)
projects/rocprofiler-systems/source/lib/rocprof-sys/library/sampling.cpp (2)
projects/rocprofiler-systems/source/lib/core/config.cpp (2)
get_tmp_file(2648-2691)get_tmp_file(2649-2649)projects/rocprofiler-systems/source/lib/core/config.hpp (1)
get_tmp_file(399-399)
projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp (1)
projects/rocprofiler-systems/source/lib/rocprof-sys-dl/dl.cpp (2)
rocprofiler_configure(1068-1074)rocprofiler_configure(1068-1070)
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp (3)
projects/rocprofiler-systems/source/bin/rocprof-sys-causal/impl.cpp (3)
update_env(325-368)update_env(326-327)update_env(948-949)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.cpp (4)
rocprofsys_attach(59-139)rocprofsys_attach(59-59)rocprofsys_detach(141-160)rocprofsys_detach(141-141)projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.hpp (2)
rocprofsys_attach(6-7)rocprofsys_detach(9-9)
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.cpp (1)
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.hpp (15)
addr(27-27)addr(28-28)addr(30-30)addr(31-31)addr(32-33)addr(52-52)addr(53-53)PTraceSession(22-22)PTraceSession(23-23)library(37-37)library(38-39)library(40-41)library(42-43)library(45-45)library(46-46)
projects/rocprofiler-systems/source/lib/core/config.hpp (2)
projects/rocprofiler-systems/source/lib/core/config.cpp (2)
settings_are_configured(201-205)settings_are_configured(202-202)projects/rocprofiler-systems/source/lib/rocprof-sys/library/thread_info.hpp (1)
_force(98-98)
projects/rocprofiler-systems/source/lib/core/config.cpp (5)
projects/rocprofiler-systems/source/lib/rocprof-sys/library/thread_info.hpp (1)
_force(98-98)projects/rocprofiler-systems/source/lib/core/config.hpp (2)
is_attach_mode(168-168)get_sampling_signals(78-78)projects/rocprofiler-systems/source/lib/rocprof-sys/library/sampling.cpp (2)
block_signals(1034-1049)block_signals(1035-1035)projects/rocprofiler-systems/source/lib/rocprof-sys/library/causal/sampling.cpp (2)
block_signals(550-557)block_signals(551-551)projects/rocprofiler-systems/source/lib/rocprof-sys/library/amd_smi.cpp (2)
get_state(238-243)get_state(239-239)
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.hpp (1)
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.cpp (2)
PTraceSession(61-64)PTraceSession(66-72)
projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp (5)
projects/rocprofiler-systems/source/lib/core/state.cpp (4)
get_state(67-71)get_state(68-68)set_state(79-93)set_state(80-80)projects/rocprofiler-systems/source/lib/rocprof-sys-dl/dl.cpp (2)
rocprofsys_finalize(648-670)rocprofsys_finalize(648-648)projects/rocprofiler-systems/source/lib/rocprof-sys/api.cpp (2)
rocprofsys_finalize(134-138)rocprofsys_finalize(135-135)projects/rocprofiler-systems/source/lib/core/config.cpp (10)
set_signal_handler(1282-1302)set_signal_handler(1283-1283)set_detach_signal_handler(1304-1324)set_detach_signal_handler(1305-1305)is_mpi_attached(1830-1835)is_mpi_attached(1831-1831)is_attach_mode(1837-1842)is_attach_mode(1838-1838)get_use_rocm(1940-1949)get_use_rocm(1941-1941)projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp (8)
setup(1898-1906)setup(1899-1899)start(1928-1940)start(1929-1929)stop(1942-1954)stop(1943-1943)shutdown(1908-1914)shutdown(1909-1909)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: ubuntu-jammy-external-rocm (g++, ON, ON, OFF, ON, Release, OFF, ON, 6.4)
- GitHub Check: opensuse (g++, 15.6, Release)
- GitHub Check: opensuse (g++, 15.5, Release)
- GitHub Check: rhel (g++, 9.3, 0.0, Release)
- GitHub Check: rhel (g++, 9.4, 0.0, Release)
- GitHub Check: rhel (g++, 8.10, 0.0, Release)
- GitHub Check: ubuntu-noble (g++, ON, Release, ON, 6.3)
- GitHub Check: ubuntu-noble (g++, ON, Release, ON, 6.4)
- GitHub Check: ubuntu-noble (g++, ON, Release, ON, 0.0)
🔇 Additional comments (23)
projects/rocprofiler-systems/source/lib/rocprof-sys-dl/dl.cpp (1)
1558-1599: Fix dangling pointer, harden dlopen/dlsym, and don’t change public API without verificationload_environment_buffer is defined as load_environment_buffer(const char*) in projects/rocprofiler-systems/source/lib/rocprof-sys-dl/dl.cpp:1362; rocprofsys_dl_attach is a public API (declaration at dl.cpp:1405, definition at dl.cpp:1559).
- Replace std::string{}.c_str() with nullptr in the rocprofsys_init call (dl.cpp:1558–1599) to avoid a dangling pointer.
- Harden the librocprofiler-register path: use "librocprofiler-register.so", check dlopen() for NULL, check dlsym() before calling the symbols, log dlerror(), and dlclose() the handle on both success and failure.
- Do not unconditionally reset rocprofsys::dl::get_inited()/get_active()/get_finied() immediately after init; either remove these resets or document explicit attach semantics and update dependent code.
- Do NOT change rocprofsys_dl_attach signature to (const void*, size_t) unless you also change load_environment_buffer and verify/update every caller (this is a public API/ABI-impacting change).
projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.hpp (1)
54-56: New public APIflush()looks good; confirm visibility/export.Header now declares
rocprofsys::rocprofiler_sdk::flush(). Implementation exists in rocprofiler-sdk.cpp. Ensure the library exports this symbol in the shared object and that downstreams don’t need extern "C".Can you confirm the symbol appears in the built .so (e.g., nm -D | rg 'rocprofsys::rocprofiler_sdk::flush')?
projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp (1)
1900-1906: Auto‑configure on first setup—sane default; verify ROCm SDK compatibility.Calling
rocprofiler_force_configure(&rocprofiler_configure)when not initialized improves attach scenarios. Sincerocprofiler_configurealready self‑guards, duplicate calls are benign. Verify these APIs exist across your supported ROCm versions to avoid dlsym failures.If you support multiple ROCm versions, please confirm availability of
rocprofiler_is_initializedandrocprofiler_force_configure.projects/rocprofiler-systems/source/lib/core/config.hpp (2)
165-169: Add deprecated alias is_attached() → is_mpi_attached()Renaming is_attached() to is_mpi_attached() is a breaking API change — add an inline forwarding shim and deprecate the old name.
File: projects/rocprofiler-systems/source/lib/core/config.hpp (lines 165-169)
namespace rocprofsys { // ... inline namespace config { // ... - bool& is_mpi_attached(); + bool& is_mpi_attached(); + // Backward-compat (deprecated) + inline bool& is_attached() { return is_mpi_attached(); }Search for external/public usages to plan deprecation messaging:
rg -nP '\bis_attached\b' -C2
57-58: Review_forceusage in configure_settings call sites
Calls toconfigure_settingsnow include a second_forceparameter defaulting tofalse. Existing invocations are:
library.cpp:466uses(true, true)library.cpp:473uses no arguments →(true, false)argparse.cpp:220uses(false, false)config.cpp(lines 109, 208) use no arguments →(true, false)Confirm any call that should re-attach or overwrite settings explicitly passes
_force = true.projects/rocprofiler-systems/source/lib/core/state.hpp (1)
41-42: Relax set_state monotonic check to allow re‑attach from Detached → ActiveState::Detached is ordered after Disabled and the current _n < get_state() check will block Detached → Active during re‑attach. Allow this limited transition by exempting specific re‑attach targets from the “no decrease” check (e.g. if cur==Detached allow _n in {PreInit, Init, Active}) and keep the existing throw for all other decreases. Location: projects/rocprofiler-systems/source/lib/core/state.cpp.
projects/rocprofiler-systems/source/lib/core/config.cpp (5)
1199-1205: Atomic storage for detach handler looks goodUsing an atomic function pointer with CAS mirrors the main signal handler pattern. LGTM.
1214-1219: Exit action now honors Active state onlyReasonable guard to avoid invoking the finalization callback if activation never occurred. LGTM.
Please confirm this does not mask needed cleanup in other non‑Active states (e.g., Detached), and that upstream timemory exit paths won’t double‑deliver signals.
1304-1324: set_detach_signal_handler mirrors main handler; OKCAS + relaxed/seq_cst fallback is consistent with the primary handler. LGTM.
1831-1841: MPI attach state split vs attach mode: fineSeparating
is_mpi_attached()fromis_attach_mode()clarifies semantics. LGTM.
208-213: Signature change: ensure header defaults or add zero-arg overload and update callersImplementation at projects/rocprofiler-systems/source/lib/core/config.cpp:208–213 defines configure_settings(bool _init, bool _force); no matching header declaration was found—add default parameter values in config.hpp (e.g., bool _init = ..., bool _force = ...) or provide a zero-arg overload and update callers so the code compiles.
projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp (4)
170-172: Registering a separate detach handler is good separation of concernsHandler slotting mirrors the exit handler. LGTM pending the signal-safety refactor.
Ensure no other code overwrites the detach handler later.
1143-1144: Skip static destruction in attach mode: goodReturning early avoids tearing down global singletons, enabling re‑attach. LGTM.
437-475: Re‑entry from Detached state: verify _once scope and re‑init safety
- Locate the declaration of _once and confirm whether it's static/global/thread_local; ensure its lifetime/scope won't suppress intended re‑attaches across PIDs.
- Verify repeated Init via configure_settings(true, true) is safe for thread pools, signal handlers, singletons, timers, and FDs — or ensure detach resets any one‑time guards.
- Inspect projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp:437–475 and search the module for _once, configure_settings, and detach_handler to confirm no hidden singletons block re‑init.
560-585: ROCm attach: setup() and start() are idempotentsetup() only calls rocprofiler_force_configure when rocprofiler_is_initialized reports not-initialized (safe to call after a prior shutdown). start() iterates contexts and calls rocprofiler_start_context only for contexts where is_initialized() && !is_active(), so repeated start() calls are harmless.
Location: projects/rocprofiler-systems/source/lib/rocprof-sys/library/rocprofiler-sdk.cpp — setup() ~line 1899, shutdown() ~line 1909, start() ~line 1929, stop() ~line 1943.
projects/rocprofiler-systems/source/lib/CMakeLists.txt (1)
77-77: Add subdirectory: OKIncluding rocprof-sys-attach in the build graph is expected for this feature. LGTM.
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/CMakeLists.txt (1)
21-21: Link sample against attach library: OKPrivate link to the new attach module is correct.
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.cpp (1)
40-41: Parsing gate tweak looks fine.Recognizing
-pto forceparse_argsis reasonable.projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.cpp (1)
118-129: Check open_library() result and bail out consistently.Current handling is OK; no action needed beyond prior fail-fast change.
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp (3)
359-364: Help text additions are fine.Descriptions for attach flags are clear.
847-855: Confirm semantics: ROCPROFSYS_ATTACH_PID set to host PID, not target.You set the env var to the controller’s PID (host), while
get_pid()stores the target PID. If the target-side expects controller PID, this is correct; otherwise, set it topid.If the target reads ROCPROFSYS_ATTACH_PID as the target PID, change:
- update_env(_env, "ROCPROFSYS_ATTACH_PID", getpid()); // Set to this PID + update_env(_env, "ROCPROFSYS_ATTACH_PID", pid); // Set to target PID
856-866: Attach duration arg wiring looks good.Env propagation and local state assignment are correct.
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/CMakeLists.txt (1)
26-26: Visibility preset: confirm the intended value.CMAKE_CXX_VISIBILITY_PRESET is commonly set to "hidden". "internal" is less common and may not be portable across compilers. Please confirm toolchain support or switch to "hidden" with explicit export annotations for public APIs.
| size_t* | ||
| get_pid() | ||
| { | ||
| static size_t _pid = 0; | ||
| return &_pid; | ||
| } | ||
|
|
||
| size_t* | ||
| get_attach_duration() | ||
| { | ||
| static size_t _v = 0; | ||
| return &_v; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pid_t for PID storage.
Avoid size_t for PIDs.
-size_t*
+pid_t*
get_pid()
{
- static size_t _pid = 0;
+ static pid_t _pid = 0;
return &_pid;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t* | |
| get_pid() | |
| { | |
| static size_t _pid = 0; | |
| return &_pid; | |
| } | |
| size_t* | |
| get_attach_duration() | |
| { | |
| static size_t _v = 0; | |
| return &_v; | |
| } | |
| pid_t* | |
| get_pid() | |
| { | |
| static pid_t _pid = 0; | |
| return &_pid; | |
| } | |
| size_t* | |
| get_attach_duration() | |
| { | |
| static size_t _v = 0; | |
| return &_v; | |
| } |
🤖 Prompt for AI Agents
In projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp around
lines 905–917, the PID storage uses size_t which is inappropriate; change
get_pid() to return a pointer to pid_t with a static pid_t _pid variable, update
the function signature accordingly, and include the appropriate header (e.g.,
<sys/types.h> or <unistd.h>) so pid_t is defined; leave get_attach_duration()
unchanged unless there is a separate requirement, and update any callers to
accept pid_t* instead of size_t*.
| int | ||
| attach(std::vector<char*> env) | ||
| { | ||
| rocprofsys_attach(*(get_pid()), env); | ||
| size_t ms = *(get_attach_duration()); | ||
| if(!ms) | ||
| { | ||
| // Press any key to detach | ||
| std::cout << "Press any key to detach from the process with PID: " << *(get_pid()) | ||
| << std::endl; | ||
| std::cin.get(); | ||
| rocprofsys_detach(*(get_pid())); | ||
| } | ||
| else | ||
| { | ||
| std::cout << "Attached. Will detach automatically in " << ms << " milliseconds\n"; | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(ms)); | ||
| rocprofsys_detach(*(get_pid())); | ||
| } | ||
| return 0; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Check attach result and short‑circuit on failure; keep types consistent.
-int
-attach(std::vector<char*> env)
+int
+attach(std::vector<char*> env)
{
- rocprofsys_attach(*(get_pid()), env);
- size_t ms = *(get_attach_duration());
+ int rc = rocprofsys_attach(*(get_pid()), env);
+ if(rc != 0) return rc;
+ size_t ms = *(get_attach_duration());
if(!ms)
{
// Press any key to detach
std::cout << "Press any key to detach from the process with PID: " << *(get_pid())
<< std::endl;
std::cin.get();
rocprofsys_detach(*(get_pid()));
}
else
{
std::cout << "Attached. Will detach automatically in " << ms << " milliseconds\n";
std::this_thread::sleep_for(std::chrono::milliseconds(ms));
rocprofsys_detach(*(get_pid()));
}
return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int | |
| attach(std::vector<char*> env) | |
| { | |
| rocprofsys_attach(*(get_pid()), env); | |
| size_t ms = *(get_attach_duration()); | |
| if(!ms) | |
| { | |
| // Press any key to detach | |
| std::cout << "Press any key to detach from the process with PID: " << *(get_pid()) | |
| << std::endl; | |
| std::cin.get(); | |
| rocprofsys_detach(*(get_pid())); | |
| } | |
| else | |
| { | |
| std::cout << "Attached. Will detach automatically in " << ms << " milliseconds\n"; | |
| std::this_thread::sleep_for(std::chrono::milliseconds(ms)); | |
| rocprofsys_detach(*(get_pid())); | |
| } | |
| return 0; | |
| } | |
| int | |
| attach(std::vector<char*> env) | |
| { | |
| int rc = rocprofsys_attach(*(get_pid()), env); | |
| if (rc != 0) return rc; | |
| size_t ms = *(get_attach_duration()); | |
| if(!ms) | |
| { | |
| // Press any key to detach | |
| std::cout << "Press any key to detach from the process with PID: " << *(get_pid()) | |
| << std::endl; | |
| std::cin.get(); | |
| rocprofsys_detach(*(get_pid())); | |
| } | |
| else | |
| { | |
| std::cout << "Attached. Will detach automatically in " << ms << " milliseconds\n"; | |
| std::this_thread::sleep_for(std::chrono::milliseconds(ms)); | |
| rocprofsys_detach(*(get_pid())); | |
| } | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp around
lines 919 to 939, capture and check the return value of rocprofsys_attach and
short‑circuit on failure (log an error and return a non‑zero exit code) instead
of always proceeding; also keep types consistent for the attach duration by
using a single unsigned integer type (e.g. uint64_t or size_t) to read
*(get_attach_duration()) and then convert/cast that value into
std::chrono::milliseconds when sleeping to avoid implicit narrowing or sign
issues.
|
|
||
| #pragma once | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include sys/types for pid_t instead of using size_t for PIDs.
Use pid_t for process IDs to avoid narrowing and platform mismatches.
-#include <cstdint>
+#include <cstdint>
+#include <sys/types.h>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <cstdint> | |
| #include <cstdint> | |
| #include <sys/types.h> |
🤖 Prompt for AI Agents
In
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.hpp
around line 25, the code currently uses size_t for process IDs and only includes
<cstdint>; replace that with the proper POSIX type by adding #include
<sys/types.h> and change any PID-related declarations, parameters and return
types from size_t to pid_t (and update any related comparisons/formatting to use
pid_t-compatible types), ensuring the header compiles on platforms where pid_t
is defined.
| size_t* | ||
| get_pid(); | ||
|
|
||
| size_t* | ||
| get_attach_duration(); | ||
|
|
||
| int | ||
| attach(std::vector<char*> env); No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Change PID-related APIs to pid_t.
Aligns types across the attach flow and with POSIX APIs (kill, ptrace).
-size_t*
-get_pid();
+pid_t*
+get_pid();
-size_t*
-get_attach_duration();
+size_t*
+get_attach_duration();
int
-attach(std::vector<char*> env);
+attach(std::vector<char*> env);If attach.hpp also uses size_t pid, update it and all call sites to pid_t for consistency.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "\n--- Show target header (first 240 lines) ---\n"
FILE="projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.hpp"
if [ -f "$FILE" ]; then
sed -n '1,240p' "$FILE" || true
else
echo "File not found: $FILE"
fi
printf "\n--- Searching repository for symbols: get_pid, get_attach_duration, pid_t, size_t <...> pid, attach(...) ---\n"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore-vcs -S '\bget_pid\b' -C3 || true
rg -n --hidden --no-ignore-vcs -S '\bget_attach_duration\b' -C3 || true
rg -n --hidden --no-ignore-vcs -S '\bpid_t\b' -C3 || true
rg -n --hidden --no-ignore-vcs -S '\bsize_t\b.*\bpid\b' -C3 || true
rg -n --hidden --no-ignore-vcs -S 'attach\s*\(' -C3 || true
rg -n --hidden --no-ignore-vcs -S 'size_t\s*\*\s*\w+\s*\(' -C3 || true
rg -n --hidden --no-ignore-vcs -S 'pid_t\s*\*\s*\w+\s*\(' -C3 || true
else
grep -RIn --line-number --exclude-dir=.git --exclude-dir=node_modules -e 'get_pid' . || true
grep -RIn --line-number --exclude-dir=.git --exclude-dir=node_modules -e 'get_attach_duration' . || true
grep -RIn --line-number --exclude-dir=.git --exclude-dir=node_modules -e 'pid_t' . || true
grep -RIn --line-number --exclude-dir=.git --exclude-dir=node_modules -E 'size_t.*\bpid\b' . || true
grep -RIn --line-number --exclude-dir=.git --exclude-dir=node_modules -e 'attach(' . || true
fi
printf "\n--- Locate any attach.hpp files ---\n"
find . -type f -iname 'attach.hpp' -print || true
printf "\n--- Summarize files referencing get_pid, get_attach_duration, or size_t <...> pid ---\n"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore-vcs -S 'get_pid|get_attach_duration|\bsize_t\b.*\bpid\b' -C2 || true
else
grep -RIn --line-number --exclude-dir=.git -E 'get_pid|get_attach_duration|size_t.*\bpid\b' . || true
fi
printf "\n--- End ---\n"Length of output: 217046
Change PID-related APIs to pid_t.
Attach/ptrace/sample currently use size_t for PIDs — change prototypes/definitions and call sites to pid_t so they match POSIX APIs (kill/ptrace) and the rest of the codebase.
- projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.hpp: change size_t* get_pid() → pid_t* get_pid(); (keep get_attach_duration as size_t*).
- projects/rocprofiler-systems/source/bin/rocprof-sys-sample/impl.cpp: change static size_t _pid → static pid_t _pid and update all *(get_pid()) assignments/uses.
- projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.hpp & attach.cpp: change extern "C" APIs rocprofsys_attach(size_t pid, ...) and rocprofsys_detach(size_t pid) → use pid_t and update all callers.
- projects/rocprofiler-systems/source/lib/rocprof-sys-attach/ptrace_session.hpp & ptrace_session.cpp: change PTraceSession constructor parameter, const pid member and get_pid() return type from size_t → pid_t; adjust PTRACE/kill call sites if necessary.
- Search the attach flow for remaining size_t PID usages and update consistently; add <sys/types.h> (or equivalent) includes where required.
🤖 Prompt for AI Agents
In
projects/rocprofiler-systems/source/bin/rocprof-sys-sample/rocprof-sys-sample.hpp
lines 67-74, change the PID-related API types from size_t* to pid_t* (keep
get_attach_duration as size_t*); update the impl.cpp to make the static _pid a
pid_t and update all *(get_pid()) assignments/uses accordingly; in
projects/rocprofiler-systems/source/lib/rocprof-sys-attach/attach.hpp &
attach.cpp change extern "C" APIs to accept/return pid_t instead of size_t and
update all callers; in ptrace_session.hpp & ptrace_session.cpp change the
PTraceSession constructor parameter, the const pid member and get_pid() return
type from size_t to pid_t and adjust PTRACE/kill call sites if needed; finally
search the attach flow for any remaining size_t PID usages and replace with
pid_t and add #include <sys/types.h> (or equivalent) where required so POSIX PID
types are used consistently.
| // Tim: This handles signals for triggering attach/detach. It prevents the process from | ||
| // being killed at the end. | ||
| void | ||
| rocprofsys_attach_detach_action(int, siginfo_t*, void*) | ||
| { | ||
| if(!config::is_attach_mode()) | ||
| { | ||
| ROCPROFSYS_BASIC_PRINT(R"( | ||
| EEEEEEEEEEEEEEEEEEEEEERRRRRRRRRRRRRRRRR RRRRRRRRRRRRRRRRR OOOOOOOOO RRRRRRRRRRRRRRRRR | ||
| E::::::::::::::::::::ER::::::::::::::::R R::::::::::::::::R OO:::::::::OO R::::::::::::::::R | ||
| E::::::::::::::::::::ER::::::RRRRRR:::::R R::::::RRRRRR:::::R OO:::::::::::::OO R::::::RRRRRR:::::R | ||
| EE::::::EEEEEEEEE::::ERR:::::R R:::::RRR:::::R R:::::RO:::::::OOO:::::::ORR:::::R R:::::R | ||
| E:::::E EEEEEE R::::R R:::::R R::::R R:::::RO::::::O O::::::O R::::R R:::::R | ||
| E:::::E R::::R R:::::R R::::R R:::::RO:::::O O:::::O R::::R R:::::R | ||
| E::::::EEEEEEEEEE R::::RRRRRR:::::R R::::RRRRRR:::::R O:::::O O:::::O R::::RRRRRR:::::R | ||
| E:::::::::::::::E R:::::::::::::RR R:::::::::::::RR O:::::O O:::::O R:::::::::::::RR | ||
| E:::::::::::::::E R::::RRRRRR:::::R R::::RRRRRR:::::R O:::::O O:::::O R::::RRRRRR:::::R | ||
| E::::::EEEEEEEEEE R::::R R:::::R R::::R R:::::RO:::::O O:::::O R::::R R:::::R | ||
| E:::::E R::::R R:::::R R::::R R:::::RO:::::O O:::::O R::::R R:::::R | ||
| E:::::E EEEEEE R::::R R:::::R R::::R R:::::RO::::::O O::::::O R::::R R:::::R | ||
| EE::::::EEEEEEEE:::::ERR:::::R R:::::RRR:::::R R:::::RO:::::::OOO:::::::ORR:::::R R:::::R | ||
| E::::::::::::::::::::ER::::::R R:::::RR::::::R R:::::R OO:::::::::::::OO R::::::R R:::::R | ||
| E::::::::::::::::::::ER::::::R R:::::RR::::::R R:::::R OO:::::::::OO R::::::R R:::::R | ||
| EEEEEEEEEEEEEEEEEEEEEERRRRRRRR RRRRRRRRRRRRRRR RRRRRRR OOOOOOOOO RRRRRRRR RRRRRR | ||
|
|
||
| Trying to attach to an uninitialized process. Only applications launched by rocprof-sys in | ||
| pre-attach mode can be attached to. | ||
|
|
||
| To launch an application in pre-attach mode, either | ||
|
|
||
| - set env var `ROCPROFSYS_ATTACH = true` or, | ||
| - launch the process with `rocprof-sys-sample --pre-attach <OPTIONS> -- <APP> <ARGS>` | ||
|
|
||
| Ignoring attach attempt and continuing... | ||
| )"); | ||
| return; | ||
| } | ||
| tim::signals::block_signals(get_sampling_signals(), | ||
| tim::signals::sigmask_scope::process); | ||
| auto _handler = get_detach_signal_handler().load(); | ||
| if(_handler) (*_handler)(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Asynchronous signal handler does non‑async‑signal‑safe work and calls into complex code
rocprofsys_attach_detach_action:
- prints ASCII art via logging (allocations, locks),
- calls
tim::signals::block_signalsandget_sampling_signals()(non‑async‑signal‑safe), - invokes a user callback (
get_detach_signal_handler()) which currently runs heavy logic (starts/stops tooling).
This can deadlock or crash under signal. In signal handlers, restrict to async‑signal‑safe ops (e.g., write(), sigatomic flags, self‑pipe). Recommend:
- Replace the body with setting a
volatile sig_atomic_tflag or writing a byte to a dedicated eventfd/self‑pipe. - Handle the actual attach/detach work on a normal thread (e.g., a small monitor thread created in
configure_signal_handleror reuse an existing runtime loop).
I can draft a self‑pipe + monitor thread patch if desired.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locating rocprofsys_attach_detach_action ==="
rg -n --hidden --no-ignore-vcs "rocprofsys_attach_detach_action" -S || true
FILE=$(rg --hidden --no-ignore-vcs "rocprofsys_attach_detach_action" -S --files-with-matches | head -n1 || true)
if [ -n "$FILE" ]; then
echo "=== printing $FILE (1180-1280) ==="
sed -n '1180,1280p' "$FILE" || true
else
echo "Function definition file not found."
fi
echo "=== searching related symbols ==="
rg -n --hidden --no-ignore-vcs "get_detach_signal_handler" -S -C3 || true
rg -n --hidden --no-ignore-vcs "set_detach_signal_handler" -S -C3 || true
rg -n --hidden --no-ignore-vcs "get_sampling_signals" -S -C3 || true
rg -n --hidden --no-ignore-vcs "tim::signals::block_signals" -S -C3 || true
rg -n --hidden --no-ignore-vcs "configure_signal_handler" -S -C3 || true
rg -n --hidden --no-ignore-vcs "sigaction" -S -C3 || true
rg -n --hidden --no-ignore-vcs "sa_sigaction" -S -C3 || true
rg -n --hidden --no-ignore-vcs "signal(" -S -C3 || true
echo "=== done ==="Length of output: 75518
Critical: unsafe SIG handler — remove non‑async‑signal‑unsafe work from rocprofsys_attach_detach_action
rocprofsys_attach_detach_action (projects/rocprofiler-systems/source/lib/core/config.cpp:1226–1264) performs non‑async‑signal‑safe operations (ROCPROFSYS_BASIC_PRINT — allocs/locks, tim::signals::block_signals(get_sampling_signals()) — non‑async‑safe, and invoking the callback from get_detach_signal_handler() which runs heavy tooling). It is installed as sa_sigaction for DETACH_SIG in configure_signal_handler (config.cpp:1367–1372).
- Fix: replace the handler body with an async‑safe notifier (volatile sig_atomic_t flag or write a byte to a dedicated self‑pipe/eventfd); do all attach/detach work on a normal thread (monitor thread created in configure_signal_handler or reuse an existing runtime loop).
- Review rocprofsys_exit_action (config.cpp:1206–1216) for similar issues.
🤖 Prompt for AI Agents
projects/rocprofiler-systems/source/lib/core/config.cpp lines 1223-1265: the SIG
handler rocprofsys_attach_detach_action performs non-async-signal-safe
operations (printing, blocking signals, invoking heavy callbacks); change it to
a minimal async-safe notifier only (set a volatile sig_atomic_t flag or write a
byte to a self-pipe/eventfd) and return immediately; then, in
configure_signal_handler create or reuse a monitor thread/loop that polls the
flag or reads the pipe/eventfd and performs the existing attach/detach work
(ROCPROFSYS_BASIC_PRINT, tim::signals::block_signals, calling the detach
handler) from the normal thread context; also update signal installation to use
the new minimal handler and similarly review/fix rocprofsys_exit_action (around
lines ~1206-1216) to follow the same pattern.
| void | ||
| load_environment_buffer(const char* environment_buffer) | ||
| { | ||
| // Environment_buffer is a null-character delimited list of name value pairs. | ||
| // Each name and value is delimited separately. | ||
| // The first 4 bytes contain an uint32_t count of pairs | ||
|
|
||
| if(!environment_buffer) | ||
| { | ||
| ROCPROFSYS_DL_LOG(1, "Attachment was invoked with no environment variables " | ||
| "provided for what to trace."); | ||
| return; | ||
| } | ||
|
|
||
| const uint32_t pair_count = *reinterpret_cast<const uint32_t*>(environment_buffer); | ||
| const char* position = environment_buffer + sizeof(uint32_t); | ||
| for(uint32_t pair_idx = 0; pair_idx < pair_count; ++pair_idx) | ||
| { | ||
| const char* name = position; | ||
| position += strlen(name) + 1; | ||
| const char* value = position; | ||
| position += strlen(value) + 1; | ||
|
|
||
| ROCPROFSYS_DL_LOG(2, "Attachment adding environment variable: %s=%s\n", name, | ||
| value); | ||
| setenv(name, value, 1); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix unbounded and potentially unaligned parsing of env buffer (OOB/UB risk).
The function trusts an attacker-controlled pointer with no length. It:
- Reinterprets an unaligned char buffer as uint32_t (UB on some archs).
- Uses strlen in a loop without any upper bound, enabling out-of-bounds reads and crashes if the buffer is malformed.
- Lacks validation against an overall buffer size.
Recommend: make the parser length-aware, copy the count via memcpy, and use strnlen with a remaining counter. Also, prefer allowing only ROCPROFSYS_* variables. Add and includes if not already present.
Apply this diff to harden the parser (paired with signature changes below):
-void
-load_environment_buffer(const char* environment_buffer)
+void
+load_environment_buffer(const void* environment_buffer, size_t buffer_len)
{
- // Environment_buffer is a null-character delimited list of name value pairs.
- // Each name and value is delimited separately.
- // The first 4 bytes contain an uint32_t count of pairs
-
- if(!environment_buffer)
- {
- ROCPROFSYS_DL_LOG(1, "Attachment was invoked with no environment variables "
- "provided for what to trace.");
- return;
- }
-
- const uint32_t pair_count = *reinterpret_cast<const uint32_t*>(environment_buffer);
- const char* position = environment_buffer + sizeof(uint32_t);
- for(uint32_t pair_idx = 0; pair_idx < pair_count; ++pair_idx)
- {
- const char* name = position;
- position += strlen(name) + 1;
- const char* value = position;
- position += strlen(value) + 1;
-
- ROCPROFSYS_DL_LOG(2, "Attachment adding environment variable: %s=%s\n", name,
- value);
- setenv(name, value, 1);
- }
+ // Environment_buffer format:
+ // [uint32_t pair_count][name'\0'][value'\0']... (repeated pair_count times)
+ if(!environment_buffer || buffer_len < sizeof(uint32_t))
+ {
+ ROCPROFSYS_DL_LOG(1, "Attachment invoked with no/short environment buffer.\n");
+ return;
+ }
+
+ uint32_t pair_count = 0;
+ std::memcpy(&pair_count, environment_buffer, sizeof(uint32_t));
+
+ const unsigned char* pos = static_cast<const unsigned char*>(environment_buffer) + sizeof(uint32_t);
+ size_t rem = buffer_len - sizeof(uint32_t);
+
+ // soft cap to avoid pathological inputs
+ const uint32_t kMaxPairs = 1024;
+ if(pair_count > kMaxPairs) pair_count = kMaxPairs;
+
+ for(uint32_t i = 0; i < pair_count; ++i)
+ {
+ size_t name_len = strnlen(reinterpret_cast<const char*>(pos), rem);
+ if(name_len >= rem) { ROCPROFSYS_DL_LOG(0, "Malformed env buffer (name).\n"); return; }
+ const char* name = reinterpret_cast<const char*>(pos);
+ pos += name_len + 1; rem -= name_len + 1;
+
+ size_t val_len = strnlen(reinterpret_cast<const char*>(pos), rem);
+ if(val_len >= rem) { ROCPROFSYS_DL_LOG(0, "Malformed env buffer (value).\n"); return; }
+ const char* value = reinterpret_cast<const char*>(pos);
+ pos += val_len + 1; rem -= val_len + 1;
+
+ // optionally: restrict to "ROCPROFSYS_*"
+ // if(strncmp(name, "ROCPROFSYS_", 11) != 0) continue;
+
+ ROCPROFSYS_DL_LOG(2, "Attachment adding environment variable: %s=%s\n", name, value);
+ setenv(name, value, 1);
+ }
}Outside this hunk: please add the required headers near the top of the file:
- #include
- #include
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| load_environment_buffer(const char* environment_buffer) | |
| { | |
| // Environment_buffer is a null-character delimited list of name value pairs. | |
| // Each name and value is delimited separately. | |
| // The first 4 bytes contain an uint32_t count of pairs | |
| if(!environment_buffer) | |
| { | |
| ROCPROFSYS_DL_LOG(1, "Attachment was invoked with no environment variables " | |
| "provided for what to trace."); | |
| return; | |
| } | |
| const uint32_t pair_count = *reinterpret_cast<const uint32_t*>(environment_buffer); | |
| const char* position = environment_buffer + sizeof(uint32_t); | |
| for(uint32_t pair_idx = 0; pair_idx < pair_count; ++pair_idx) | |
| { | |
| const char* name = position; | |
| position += strlen(name) + 1; | |
| const char* value = position; | |
| position += strlen(value) + 1; | |
| ROCPROFSYS_DL_LOG(2, "Attachment adding environment variable: %s=%s\n", name, | |
| value); | |
| setenv(name, value, 1); | |
| } | |
| } | |
| void | |
| load_environment_buffer(const void* environment_buffer, size_t buffer_len) | |
| { | |
| // Environment_buffer format: | |
| // [uint32_t pair_count][name'\0'][value'\0']... (repeated pair_count times) | |
| if(!environment_buffer || buffer_len < sizeof(uint32_t)) | |
| { | |
| ROCPROFSYS_DL_LOG(1, "Attachment invoked with no/short environment buffer.\n"); | |
| return; | |
| } | |
| uint32_t pair_count = 0; | |
| std::memcpy(&pair_count, environment_buffer, sizeof(uint32_t)); | |
| const unsigned char* pos = static_cast<const unsigned char*>(environment_buffer) + sizeof(uint32_t); | |
| size_t rem = buffer_len - sizeof(uint32_t); | |
| // soft cap to avoid pathological inputs | |
| const uint32_t kMaxPairs = 1024; | |
| if(pair_count > kMaxPairs) pair_count = kMaxPairs; | |
| for(uint32_t i = 0; i < pair_count; ++i) | |
| { | |
| size_t name_len = strnlen(reinterpret_cast<const char*>(pos), rem); | |
| if(name_len >= rem) { ROCPROFSYS_DL_LOG(0, "Malformed env buffer (name).\n"); return; } | |
| const char* name = reinterpret_cast<const char*>(pos); | |
| pos += name_len + 1; rem -= name_len + 1; | |
| size_t val_len = strnlen(reinterpret_cast<const char*>(pos), rem); | |
| if(val_len >= rem) { ROCPROFSYS_DL_LOG(0, "Malformed env buffer (value).\n"); return; } | |
| const char* value = reinterpret_cast<const char*>(pos); | |
| pos += val_len + 1; rem -= val_len + 1; | |
| // optionally: restrict to "ROCPROFSYS_*" | |
| // if(strncmp(name, "ROCPROFSYS_", 11) != 0) continue; | |
| ROCPROFSYS_DL_LOG(2, "Attachment adding environment variable: %s=%s\n", name, value); | |
| setenv(name, value, 1); | |
| } | |
| } |
| int rocprofsys_dl_attach(const char* env_buff) ROCPROFSYS_PUBLIC_API; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Public API should carry buffer length for safe parsing.
Expose the length in the new API. This is a new symbol; best to get it right now.
- int rocprofsys_dl_attach(const char* env_buff) ROCPROFSYS_PUBLIC_API;
+ int rocprofsys_dl_attach(const void* env_buff, size_t env_len) ROCPROFSYS_PUBLIC_API;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int rocprofsys_dl_attach(const char* env_buff) ROCPROFSYS_PUBLIC_API; | |
| int rocprofsys_dl_attach(const void* env_buff, size_t env_len) ROCPROFSYS_PUBLIC_API; |
🤖 Prompt for AI Agents
In projects/rocprofiler-systems/source/lib/rocprof-sys-dl/dl.cpp around lines
1405-1406, the public API rocprofsys_dl_attach(const char* env_buff) must accept
a buffer length to avoid unsafe parsing; change the declaration to include a
size_t/env_buff_len parameter, update the corresponding implementation and any
internal helpers to use the provided length instead of relying on
NUL-termination, update all callers to pass the buffer length, bump the API
symbol/version as appropriate (new symbol), and update any header(s) and API
docs/tests to reflect the new signature.
| // Tim: Handles attach/detach. This replaces finalization handler if dl in initialized in | ||
| // attach mode. | ||
| void | ||
| detach_handler() | ||
| { | ||
| if(get_state() < State::Active) | ||
| { | ||
| ROCPROFSYS_VERBOSE_F(1, "ATTACH ACTIVE\n"); | ||
| rocprofsys_init_tooling_hidden(); | ||
| return; | ||
| } | ||
| rocprofsys_finalize(); | ||
| } | ||
| auto |
There was a problem hiding this comment.
detach_handler runs heavy logic in signal context
This is invoked from the SA_SIGINFO handler via get_detach_signal_handler(). It calls rocprofsys_init_tooling_hidden()/rocprofsys_finalize(), both far beyond async‑signal‑safe scope. This risks deadlocks and undefined behavior.
- Convert the signal path to a notifier (sigatomic flag or self‑pipe).
- Move this function’s body to a normal thread that reacts to the notification.
I can provide a self‑pipe monitor integrated withensure_finalization()on request.
| if(_is_attach) | ||
| { | ||
| // Send signal to controller to finish cleaning up | ||
| const char* NOTIFY_PIPE_PATH = "/tmp/rocprofsys_detach_pipe"; // Hardcoded path | ||
| int pipe_fd = open(NOTIFY_PIPE_PATH, O_WRONLY); | ||
| if(pipe_fd != -1) | ||
| { | ||
| ssize_t bytes = | ||
| write(pipe_fd, "D", 1); // Write one byte to unblock the controller | ||
| if(bytes == -1) | ||
| { | ||
| ROCPROFSYS_VERBOSE_F(0, "Failed to write to detach confirmation pipe"); | ||
| } | ||
| close(pipe_fd); | ||
| } | ||
| else | ||
| { | ||
| // Log an error if possible | ||
| fprintf(stderr, "[TARGET ERROR] Could not open notify pipe.\n"); | ||
| } | ||
| set_state(State::Detached); | ||
| ROCPROFSYS_VERBOSE_F(1, "Resuming normal execution.\n"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Hardcoded FIFO path under /tmp is unsafe and brittle
/tmp/rocprofsys_detach_pipe is vulnerable to TOCTOU/symlink issues and cross‑user collisions. Use an env‑provided path, validate it’s a FIFO, and open with O_NOFOLLOW|O_CLOEXEC.
- const char* NOTIFY_PIPE_PATH = "/tmp/rocprofsys_detach_pipe"; // Hardcoded path
- int pipe_fd = open(NOTIFY_PIPE_PATH, O_WRONLY);
+ const char* envp = getenv("ROCPROFSYS_DETACH_PIPE");
+ const char* NOTIFY_PIPE_PATH = (envp && *envp) ? envp : nullptr;
+ if(!NOTIFY_PIPE_PATH) {
+ ROCPROFSYS_VERBOSE_F(0, "No detach notify pipe specified (ROCPROFSYS_DETACH_PIPE unset)\n");
+ }
+ int pipe_fd = (NOTIFY_PIPE_PATH) ? open(NOTIFY_PIPE_PATH, O_WRONLY | O_CLOEXEC | O_NOFOLLOW) : -1;
if(pipe_fd != -1)
{
+ struct stat st{};
+ if(fstat(pipe_fd, &st) == 0 && S_ISFIFO(st.st_mode)) {
ssize_t bytes =
write(pipe_fd, "D", 1); // Write one byte to unblock the controller
if(bytes == -1)
{
ROCPROFSYS_VERBOSE_F(0, "Failed to write to detach confirmation pipe");
}
close(pipe_fd);
+ } else {
+ ROCPROFSYS_VERBOSE_F(0, "Notify path is not a FIFO\n");
+ close(pipe_fd);
+ }
}
else
{
// Log an error if possible
fprintf(stderr, "[TARGET ERROR] Could not open notify pipe.\n");
}Also consider a per‑session unique path (e.g., include PID) created by the controller with 0600 perms.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(_is_attach) | |
| { | |
| // Send signal to controller to finish cleaning up | |
| const char* NOTIFY_PIPE_PATH = "/tmp/rocprofsys_detach_pipe"; // Hardcoded path | |
| int pipe_fd = open(NOTIFY_PIPE_PATH, O_WRONLY); | |
| if(pipe_fd != -1) | |
| { | |
| ssize_t bytes = | |
| write(pipe_fd, "D", 1); // Write one byte to unblock the controller | |
| if(bytes == -1) | |
| { | |
| ROCPROFSYS_VERBOSE_F(0, "Failed to write to detach confirmation pipe"); | |
| } | |
| close(pipe_fd); | |
| } | |
| else | |
| { | |
| // Log an error if possible | |
| fprintf(stderr, "[TARGET ERROR] Could not open notify pipe.\n"); | |
| } | |
| set_state(State::Detached); | |
| ROCPROFSYS_VERBOSE_F(1, "Resuming normal execution.\n"); | |
| } | |
| if(_is_attach) | |
| { | |
| // Send signal to controller to finish cleaning up | |
| const char* envp = getenv("ROCPROFSYS_DETACH_PIPE"); | |
| const char* NOTIFY_PIPE_PATH = (envp && *envp) ? envp : nullptr; | |
| if(!NOTIFY_PIPE_PATH) { | |
| ROCPROFSYS_VERBOSE_F(0, "No detach notify pipe specified (ROCPROFSYS_DETACH_PIPE unset)\n"); | |
| } | |
| int pipe_fd = (NOTIFY_PIPE_PATH) ? open(NOTIFY_PIPE_PATH, O_WRONLY | O_CLOEXEC | O_NOFOLLOW) : -1; | |
| if(pipe_fd != -1) | |
| { | |
| struct stat st{}; | |
| if(fstat(pipe_fd, &st) == 0 && S_ISFIFO(st.st_mode)) { | |
| ssize_t bytes = | |
| write(pipe_fd, "D", 1); // Write one byte to unblock the controller | |
| if(bytes == -1) | |
| { | |
| ROCPROFSYS_VERBOSE_F(0, "Failed to write to detach confirmation pipe"); | |
| } | |
| close(pipe_fd); | |
| } else { | |
| ROCPROFSYS_VERBOSE_F(0, "Notify path is not a FIFO\n"); | |
| close(pipe_fd); | |
| } | |
| } | |
| else | |
| { | |
| // Log an error if possible | |
| fprintf(stderr, "[TARGET ERROR] Could not open notify pipe.\n"); | |
| } | |
| set_state(State::Detached); | |
| ROCPROFSYS_VERBOSE_F(1, "Resuming normal execution.\n"); | |
| } |
🤖 Prompt for AI Agents
In projects/rocprofiler-systems/source/lib/rocprof-sys/library.cpp around lines
1115-1138, replace the hardcoded "/tmp/rocprofsys_detach_pipe" usage with an
environment- or controller-supplied path (fallback to a safe default only if
unavoidable); before opening, validate the path length and sanitize input, then
open the file with O_WRONLY | O_NOFOLLOW | O_CLOEXEC and check the resulting fd;
immediately fstat the fd and verify S_ISFIFO(st_mode) (and optionally that
st_mode & 0177 == 0 or permission bits match 0600) to avoid symlink/TOCTOU and
cross-user issues; handle all error cases with clear logging, do the single-byte
write if the checks pass, and prefer a per-session unique FIFO name (e.g.,
include controller PID or session id created by the controller with 0600 perms)
rather than a global /tmp filename.
| if(!_file) | ||
| { | ||
| // Initialize the offload file. This is necessary in the case of re-attach | ||
| auto& _tmp_v = get_offload_file(); | ||
| _tmp_v = config::get_tmp_file("sampling"); | ||
| auto _success = _tmp_v->open(); | ||
| ROCPROFSYS_CI_FAIL(!_success, | ||
| "Error opening sampling offload temporary file '%s'\n", | ||
| _tmp_v->filename.c_str()); | ||
| } | ||
| if(_file && *_file) | ||
| { | ||
| // Ensure that the offload file is open | ||
| _sampler->set_offload(&offload_buffer); | ||
| }; |
There was a problem hiding this comment.
Fix re-attach offload init: update local handle, clear stale positions, and avoid noop condition.
_fileis not updated after reinitializingget_offload_file(), soset_offload()may never be enabled on re-attach.- Old
offload_seq_datapositions persist across runs and will point into a new file (corruption risk). - Trailing
};after the block is a stray semicolon.
Apply:
- auto _file = get_offload_file();
- if(!_file)
- {
- // Initialize the offload file. This is necessary in the case of re-attach
- auto& _tmp_v = get_offload_file();
- _tmp_v = config::get_tmp_file("sampling");
- auto _success = _tmp_v->open();
- ROCPROFSYS_CI_FAIL(!_success,
- "Error opening sampling offload temporary file '%s'\n",
- _tmp_v->filename.c_str());
- }
- if(_file && *_file)
- {
- // Ensure that the offload file is open
- _sampler->set_offload(&offload_buffer);
- };
+ auto _file = get_offload_file();
+ if(!_file)
+ {
+ // Initialize the offload file for re-attach
+ auto& _of = get_offload_file();
+ _of = config::get_tmp_file("sampling");
+ auto ok = _of->open();
+ ROCPROFSYS_CI_FAIL(!ok,
+ "Error opening sampling offload temporary file '%s'\n",
+ _of->filename.c_str());
+ // stale offsets from prior runs are invalid for a new file
+ offload_seq_data.clear();
+ _file = _of;
+ }
+ if(_file && _file->stream.is_open())
+ {
+ _sampler->set_offload(&offload_buffer);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(!_file) | |
| { | |
| // Initialize the offload file. This is necessary in the case of re-attach | |
| auto& _tmp_v = get_offload_file(); | |
| _tmp_v = config::get_tmp_file("sampling"); | |
| auto _success = _tmp_v->open(); | |
| ROCPROFSYS_CI_FAIL(!_success, | |
| "Error opening sampling offload temporary file '%s'\n", | |
| _tmp_v->filename.c_str()); | |
| } | |
| if(_file && *_file) | |
| { | |
| // Ensure that the offload file is open | |
| _sampler->set_offload(&offload_buffer); | |
| }; | |
| auto _file = get_offload_file(); | |
| if(!_file) | |
| { | |
| // Initialize the offload file for re-attach | |
| auto& _of = get_offload_file(); | |
| _of = config::get_tmp_file("sampling"); | |
| auto ok = _of->open(); | |
| ROCPROFSYS_CI_FAIL(!ok, | |
| "Error opening sampling offload temporary file '%s'\n", | |
| _of->filename.c_str()); | |
| // stale offsets from prior runs are invalid for a new file | |
| offload_seq_data.clear(); | |
| _file = _of; | |
| } | |
| if(_file && _file->stream.is_open()) | |
| { | |
| _sampler->set_offload(&offload_buffer); | |
| } |
🤖 Prompt for AI Agents
In projects/rocprofiler-systems/source/lib/rocprof-sys/library/sampling.cpp
around lines 825-839, the re-attach path doesn't update the local _file handle
after recreating the offload file, leaves stale offload_seq_data positions from
a prior file, and contains a stray trailing semicolon; to fix, after assigning
get_offload_file() to the temporary (_tmp_v) update the local _file to point at
the new file handle so subsequent checks succeed, reset/clear any persistent
offload position/state (e.g., zero or clear offload_seq_data/offload_buffer
position indexes) so they don't reference offsets in the new file, ensure
set_offload(&offload_buffer) is called when the file is open, and remove the
trailing stray semicolon from the closing block.
2 participants
Summary by CodeRabbit