[Comgr] Open time-statistics log with O_NOFOLLOW#3209
Open
lamb-j wants to merge 1 commit into
Open
Conversation
The AMD_COMGR_TIME_STATISTICS log path can be caller-influenced via AMD_COMGR_REDIRECT_LOGS, or default to a CWD-relative PerfStatsLog.txt. Opening it without O_NOFOLLOW allows a pre-planted symlink at that path to be followed and truncated with the process's privileges. Open the file directly with O_NOFOLLOW on POSIX and wrap the descriptor in a raw_fd_ostream. Windows retains the existing path-based open. ISSUE ID: ROCM-26581
chinmaydd
reviewed
Jul 6, 2026
| // followed. The path may be caller-influenced (AMD_COMGR_REDIRECT_LOGS) or | ||
| // default to a CWD-relative file, so refuse to write through a symlink. | ||
| int FD = ::open(LogFile.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, | ||
| 0644); |
There was a problem hiding this comment.
Can we not hardcode the file permissions ? Seems more permissive than the default
chinmaydd
reviewed
Jul 6, 2026
Comment on lines
+50
to
+54
| if (FD < 0) { | ||
| EC = std::error_code(errno, std::generic_category()); | ||
| } else { | ||
| LogF.reset(new (std::nothrow) | ||
| llvm::raw_fd_ostream(FD, /*shouldClose=*/true)); |
chinmaydd
reviewed
Jul 6, 2026
Comment on lines
+50
to
+55
| if (FD < 0) { | ||
| EC = std::error_code(errno, std::generic_category()); | ||
| } else { | ||
| LogF.reset(new (std::nothrow) | ||
| llvm::raw_fd_ostream(FD, /*shouldClose=*/true)); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (FD < 0) { | |
| EC = std::error_code(errno, std::generic_category()); | |
| } else { | |
| LogF.reset(new (std::nothrow) | |
| llvm::raw_fd_ostream(FD, /*shouldClose=*/true)); | |
| } | |
| if (FD < 0) { | |
| EC = std::error_code(errno, std::generic_category()); | |
| } else { | |
| LogF.reset(new (std::nothrow) llvm::raw_fd_ostream(FD, /*shouldClose=*/true)); | |
| if (!LogF) { | |
| ::close(FD); | |
| EC = std::make_error_code(std::errc::not_enough_memory); | |
| } | |
| } |
There was a problem hiding this comment.
While we are at it, this fixes the FD leak if the new fails
There was a problem hiding this comment.
We can also probably merge the #else block below into this one
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
AMD_COMGR_TIME_STATISTICSperf log is opened withraw_fd_ostream(path, OF_Text)(i.e.O_CREAT|O_WRONLY|O_TRUNC, noO_NOFOLLOW). The path can be caller-influenced viaAMD_COMGR_REDIRECT_LOGS, or defaults to a CWD-relativePerfStatsLog.txt. A symlink pre-planted at that path would be followed and the target truncated with the process's privileges.This opens the file directly with
O_NOFOLLOWon POSIX and wraps the descriptor in araw_fd_ostream. Windows keeps the existing path-based open (O_NOFOLLOWhas no equivalent there).Note: this is an opt-in diagnostic feature (off unless the env var is set), so severity is low; this is defensive hardening. The sibling
AMD_COMGR_SAVE_TEMPSpath flagged by the same scan finding already usesfs::createUniqueDirectory(mkdtemp-equivalent) and needs no change.ISSUE ID: ROCM-26581