From 6feb31a166b689e99edfbbd539c420f02a239aff Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 27 Mar 2026 20:26:54 -0600 Subject: [PATCH 1/6] Coverity: Set umask 0077 before creating tmp files Coverity points out that it's a potential security problem if you create tmp files with group and world permissions. Adds umask calls to state_mt and the common test header. --- test/amd_detail/state_mt.cpp | 6 ++++++ test/common/test-common.h | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/test/amd_detail/state_mt.cpp b/test/amd_detail/state_mt.cpp index 79974108..644b7512 100644 --- a/test/amd_detail/state_mt.cpp +++ b/test/amd_detail/state_mt.cpp @@ -49,6 +49,10 @@ static atomic run_flag{true}; static int make_temp_file() { + // Security analyzers will be sad if you don't set umask 0077 + // before creating temporary files + mode_t old_umask = umask(S_IRWXG | S_IRWXO); + // Create a unique temporary file char pathname[]{"state_mt_tmp.XXXXXX"}; int fd{mkstemp(pathname)}; @@ -57,6 +61,8 @@ make_temp_file() exit(EXIT_FAILURE); } + umask(old_umask); + // Ensure the file is deleted when fd is closed if (-1 == unlink(pathname)) { cerr << "unlink() failed! " << strerror(errno) << endl; diff --git a/test/common/test-common.h b/test/common/test-common.h index 95debab6..440c7098 100644 --- a/test/common/test-common.h +++ b/test/common/test-common.h @@ -12,6 +12,7 @@ #include #include #include +#include #include constexpr hipFileError_t @@ -62,11 +63,17 @@ struct Tmpfile { Tmpfile(std::string directory) : path{directory} { + // Security analyzers will be sad if you don't set umask 0077 + // before creating temporary files + mode_t old_umask = umask(S_IRWXG | S_IRWXO); + path += "/hipFile.XXXXXX"; if ((fd = mkstemp(path.data())) == -1) { throw std::runtime_error("Could not create temporary file"); } + umask(old_umask); + #ifdef __HIP_PLATFORM_AMD__ if (unlink(path.c_str()) == -1) { throw std::runtime_error("Could not unlink temporary file"); From 5340f50ed72005ecb9bace690b128874287906f7 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Wed, 1 Apr 2026 10:50:07 -0600 Subject: [PATCH 2/6] Add sys/stat.h to state_mt.cpp --- test/amd_detail/state_mt.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/test/amd_detail/state_mt.cpp b/test/amd_detail/state_mt.cpp index 644b7512..39748f57 100644 --- a/test/amd_detail/state_mt.cpp +++ b/test/amd_detail/state_mt.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include using namespace hipFile; From 10646e6fd925388577b1c13256256baf47893109 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Wed, 1 Apr 2026 11:11:02 -0600 Subject: [PATCH 3/6] Call umask before throwing in test-common.h --- test/common/test-common.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/test-common.h b/test/common/test-common.h index 440c7098..856900bf 100644 --- a/test/common/test-common.h +++ b/test/common/test-common.h @@ -69,6 +69,7 @@ struct Tmpfile { path += "/hipFile.XXXXXX"; if ((fd = mkstemp(path.data())) == -1) { + umask(old_umask); throw std::runtime_error("Could not create temporary file"); } From 92ed63bf019ac27bebb76e743b7d75e971be032b Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Wed, 1 Apr 2026 11:24:27 -0600 Subject: [PATCH 4/6] Better umask handling in state_mt tester --- test/amd_detail/state_mt.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/amd_detail/state_mt.cpp b/test/amd_detail/state_mt.cpp index 39748f57..9fbdadf2 100644 --- a/test/amd_detail/state_mt.cpp +++ b/test/amd_detail/state_mt.cpp @@ -50,10 +50,6 @@ static atomic run_flag{true}; static int make_temp_file() { - // Security analyzers will be sad if you don't set umask 0077 - // before creating temporary files - mode_t old_umask = umask(S_IRWXG | S_IRWXO); - // Create a unique temporary file char pathname[]{"state_mt_tmp.XXXXXX"}; int fd{mkstemp(pathname)}; @@ -62,8 +58,6 @@ make_temp_file() exit(EXIT_FAILURE); } - umask(old_umask); - // Ensure the file is deleted when fd is closed if (-1 == unlink(pathname)) { cerr << "unlink() failed! " << strerror(errno) << endl; @@ -376,6 +370,15 @@ thread_function(int id) int main() { + // Security analyzers will be sad if you don't set umask 0077 + // before creating temporary files + // + // Doing this here to avoid inadvertently synchronizing files if we + // added a mutex in the temp file function + // + // Ignoring return type since we never switch it back + (void)umask(S_IRWXG | S_IRWXO); + struct rlimit nofile; if (-1 == getrlimit(RLIMIT_NOFILE, &nofile)) { cerr << "getrlimit() failed! " << strerror(errno) << endl; From 3b0e5658af21a863c465de1ce65c039ffe8484f3 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Wed, 1 Apr 2026 11:56:39 -0600 Subject: [PATCH 5/6] Add a note that the Tmpfile ctor is not thread-safe --- test/common/test-common.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/common/test-common.h b/test/common/test-common.h index 856900bf..a3fef237 100644 --- a/test/common/test-common.h +++ b/test/common/test-common.h @@ -61,6 +61,10 @@ struct Tmpfile { { } + /*! + * \warning The constructor is NOT thread-safe and can result in + * interleaved calls to `umask()` + */ Tmpfile(std::string directory) : path{directory} { // Security analyzers will be sad if you don't set umask 0077 From 72d534d638d10182abade6b632342da75d3b18c5 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Wed, 1 Apr 2026 12:07:51 -0600 Subject: [PATCH 6/6] Close fd on unlink failure --- test/common/test-common.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/test-common.h b/test/common/test-common.h index a3fef237..97f042b0 100644 --- a/test/common/test-common.h +++ b/test/common/test-common.h @@ -81,6 +81,7 @@ struct Tmpfile { #ifdef __HIP_PLATFORM_AMD__ if (unlink(path.c_str()) == -1) { + close(fd); throw std::runtime_error("Could not unlink temporary file"); } #endif