From 2924dd3abb0dd5642b09430654a33788e397c9da Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 27 Mar 2026 19:43:04 -0600 Subject: [PATCH 1/6] Coverity: Mark use-after-move as intentional Coverity complains about using moved hipFile descriptors in some tests that check for behavior after destruction. These tests could be improved, but for now we'll mark the behavior as intentional (as we do for clang). --- test/amd_detail/file-descriptor.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/amd_detail/file-descriptor.cpp b/test/amd_detail/file-descriptor.cpp index d67ee06b..61a5b7ea 100644 --- a/test/amd_detail/file-descriptor.cpp +++ b/test/amd_detail/file-descriptor.cpp @@ -41,6 +41,8 @@ TEST_F(HipFileFileDescriptor, ManagedFileDescriptorMoveCopyConstructor) { auto fd_a{FileDescriptor::make_managed(42)}; auto fd_b{std::move(fd_a)}; + + // coverity[USE_AFTER_MOVE] ASSERT_EQ(fd_a.get(), -1); // NOLINT(clang-analyzer-cplusplus.Move) ASSERT_EQ(fd_b.get(), 42); EXPECT_CALL(msys, close(42)); @@ -50,6 +52,8 @@ TEST_F(HipFileFileDescriptor, UnmanagedFileDescriptorMoveCopyConstructor) { auto fd_a{FileDescriptor::make_unmanaged(42)}; auto fd_b{std::move(fd_a)}; + + // coverity[USE_AFTER_MOVE] ASSERT_EQ(fd_a.get(), -1); // NOLINT(clang-analyzer-cplusplus.Move) ASSERT_EQ(fd_b.get(), 42); } @@ -60,6 +64,8 @@ TEST_F(HipFileFileDescriptor, ManagedFileDescriptorToManagedFileDescriptorMoveAs auto fd_b{FileDescriptor::make_managed(12)}; EXPECT_CALL(msys, close(12)); fd_b = std::move(fd_a); + + // coverity[USE_AFTER_MOVE] ASSERT_EQ(fd_a.get(), -1); // NOLINT(clang-analyzer-cplusplus.Move) ASSERT_EQ(fd_b.get(), 42); EXPECT_CALL(msys, close(42)); @@ -70,6 +76,8 @@ TEST_F(HipFileFileDescriptor, ManagedFileDescriptorToUnmanagedFileDescriptorMove auto fd_a{FileDescriptor::make_managed(42)}; auto fd_b{FileDescriptor::make_unmanaged(12)}; fd_b = std::move(fd_a); + + // coverity[USE_AFTER_MOVE] ASSERT_EQ(fd_a.get(), -1); // NOLINT(clang-analyzer-cplusplus.Move) ASSERT_EQ(fd_b.get(), 42); EXPECT_CALL(msys, close(42)); @@ -81,6 +89,8 @@ TEST_F(HipFileFileDescriptor, UnmanagedFileDescriptorToManagedFileDescriptorMove auto fd_b{FileDescriptor::make_managed(12)}; EXPECT_CALL(msys, close(12)); fd_b = std::move(fd_a); + + // coverity[USE_AFTER_MOVE] ASSERT_EQ(fd_a.get(), -1); // NOLINT(clang-analyzer-cplusplus.Move) ASSERT_EQ(fd_b.get(), 42); } @@ -90,6 +100,8 @@ TEST_F(HipFileFileDescriptor, UnmanagedFileDescriptorToUnmanagedFileDescriptorMo auto fd_a{FileDescriptor::make_unmanaged(42)}; auto fd_b{FileDescriptor::make_unmanaged(12)}; fd_b = std::move(fd_a); + + // coverity[USE_AFTER_MOVE] ASSERT_EQ(fd_a.get(), -1); // NOLINT(clang-analyzer-cplusplus.Move) ASSERT_EQ(fd_b.get(), 42); } From bc3fc4bfdf4f4dd9c8788fcf7b53fd8c2a97f961 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 27 Mar 2026 19:59:13 -0600 Subject: [PATCH 2/6] Coverity: Initialize msghdr struct in stats.cpp Coverity complains about the msghdr struct not having the msg_flags field set, so it will contain garbage. * Explicitly set the msg_flags field to 0 * Defensively initialize the msghdr structs using {} --- src/amd_detail/stats.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/amd_detail/stats.cpp b/src/amd_detail/stats.cpp index 214e1a30..90323459 100644 --- a/src/amd_detail/stats.cpp +++ b/src/amd_detail/stats.cpp @@ -22,8 +22,8 @@ sendFd(int sock, int fd) noexcept { int data{1}; iovec iov{&data, sizeof(data)}; - msghdr msgh; - cmsghdr *cmsgp; + msghdr msgh{}; + cmsghdr *cmsgp = nullptr; union { char buff[CMSG_SPACE(sizeof(int))]; @@ -36,6 +36,7 @@ sendFd(int sock, int fd) noexcept msgh.msg_iovlen = 1; msgh.msg_control = controlMsg.buff; msgh.msg_controllen = sizeof(controlMsg.buff); + msgh.msg_flags = 0; cmsgp = CMSG_FIRSTHDR(&msgh); cmsgp->cmsg_level = SOL_SOCKET; @@ -54,8 +55,8 @@ recvFd(int sockfd) noexcept { int data, fd; iovec iov{&data, sizeof(data)}; - msghdr msgh; - cmsghdr *cmsgp; + msghdr msgh{}; + cmsghdr *cmsgp = nullptr; union { char buff[CMSG_SPACE(sizeof(int))]; @@ -68,6 +69,7 @@ recvFd(int sockfd) noexcept msgh.msg_iovlen = 1; msgh.msg_control = controlMsg.buff; msgh.msg_controllen = sizeof(controlMsg.buff); + msgh.msg_flags = 0; if (recvmsg(sockfd, &msgh, 0) == -1) return -1; From 4dba452cbac1f8b8f52df3ad4d4dabdcea7072a9 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 27 Mar 2026 20:01:28 -0600 Subject: [PATCH 3/6] Coverity: Close socket in stats.cpp on errors Coverity complains about an unclosed socket in stats.cpp under certain error conditions. Fixed by setting a common exit point where the socket is always closed if it's not -1. --- src/amd_detail/stats.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/amd_detail/stats.cpp b/src/amd_detail/stats.cpp index 90323459..bc225da8 100644 --- a/src/amd_detail/stats.cpp +++ b/src/amd_detail/stats.cpp @@ -134,15 +134,15 @@ StatsServer::threadFn() int sock{socket(AF_UNIX, SOCK_STREAM, 0)}; pid_t pid{getpid()}; if (sock == -1) { - return; + goto out; } sockaddr_un addr; populateSocketAddr(addr, pid); if (bind(sock, reinterpret_cast(&addr), sizeof(addr)) == -1) { - return; + goto out; } if (listen(sock, 64) == -1) { - return; + goto out; } while (true) { pollfd pfd[2]{{sock, POLLIN, 0}, {m_efd.get(), POLLIN, 0}}; @@ -160,7 +160,11 @@ StatsServer::threadFn() break; } } - close(sock); + +out: + if (sock != -1) { + close(sock); + } } StatsClient::StatsClient(pid_t p) From 6abc89897e52398da434c6f1a175bba185fc12c8 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Mon, 30 Mar 2026 17:04:44 -0600 Subject: [PATCH 4/6] Initialize controlMsg and cmsgp * controlMsg union was not initialized * cmsgp switched from `= nullptr` to `{}` for uniformity --- src/amd_detail/stats.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/amd_detail/stats.cpp b/src/amd_detail/stats.cpp index bc225da8..8b0c6cd7 100644 --- a/src/amd_detail/stats.cpp +++ b/src/amd_detail/stats.cpp @@ -23,12 +23,12 @@ sendFd(int sock, int fd) noexcept int data{1}; iovec iov{&data, sizeof(data)}; msghdr msgh{}; - cmsghdr *cmsgp = nullptr; + cmsghdr *cmsgp{}; union { char buff[CMSG_SPACE(sizeof(int))]; cmsghdr align; - } controlMsg; + } controlMsg{}; msgh.msg_name = nullptr; msgh.msg_namelen = 0; @@ -56,12 +56,12 @@ recvFd(int sockfd) noexcept int data, fd; iovec iov{&data, sizeof(data)}; msghdr msgh{}; - cmsghdr *cmsgp = nullptr; + cmsghdr *cmsgp{}; union { char buff[CMSG_SPACE(sizeof(int))]; cmsghdr align; - } controlMsg; + } controlMsg{}; msgh.msg_name = nullptr; msgh.msg_namelen = 0; From 260084901735d9a9635f8a5bfe6f2e687d321101 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Mon, 30 Mar 2026 17:08:11 -0600 Subject: [PATCH 5/6] Handle unchecked poll(2) return value * Continue when -1 && errno == EINTR * Break when -1 otherwise * Continue when 0 --- src/amd_detail/stats.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/amd_detail/stats.cpp b/src/amd_detail/stats.cpp index 8b0c6cd7..a26eaa79 100644 --- a/src/amd_detail/stats.cpp +++ b/src/amd_detail/stats.cpp @@ -146,7 +146,23 @@ StatsServer::threadFn() } while (true) { pollfd pfd[2]{{sock, POLLIN, 0}, {m_efd.get(), POLLIN, 0}}; - poll(&pfd[0], 2, -1); + + int ret = poll(&pfd[0], 2, -1); + + if (ret == 0) { + continue; + } + else if (ret < 0) { + if (errno == EINTR) { + // Signal interrupt + continue; + } + else { + // Badness + break; + } + } + if (pfd[0].revents & POLLIN) { socklen_t addrlen{sizeof(addr)}; int conn{accept(sock, reinterpret_cast(&addr), &addrlen)}; From e4d6a9159f56374aeb1d513e0fb043e6e8725337 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Mon, 30 Mar 2026 17:14:16 -0600 Subject: [PATCH 6/6] Use RAII for socket via FileDescriptor --- src/amd_detail/stats.cpp | 25 ++++++++++--------------- test/amd_detail/stats.cpp | 2 +- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/amd_detail/stats.cpp b/src/amd_detail/stats.cpp index a26eaa79..466fb840 100644 --- a/src/amd_detail/stats.cpp +++ b/src/amd_detail/stats.cpp @@ -131,21 +131,21 @@ StatsServer::statsDeleter(Stats *s) void StatsServer::threadFn() { - int sock{socket(AF_UNIX, SOCK_STREAM, 0)}; - pid_t pid{getpid()}; - if (sock == -1) { - goto out; + FileDescriptor sock{FileDescriptor::make_managed(socket(AF_UNIX, SOCK_STREAM, 0))}; + pid_t pid{getpid()}; + if (sock.get() == -1) { + return; } sockaddr_un addr; populateSocketAddr(addr, pid); - if (bind(sock, reinterpret_cast(&addr), sizeof(addr)) == -1) { - goto out; + if (bind(sock.get(), reinterpret_cast(&addr), sizeof(addr)) == -1) { + return; } - if (listen(sock, 64) == -1) { - goto out; + if (listen(sock.get(), 64) == -1) { + return; } while (true) { - pollfd pfd[2]{{sock, POLLIN, 0}, {m_efd.get(), POLLIN, 0}}; + pollfd pfd[2]{{sock.get(), POLLIN, 0}, {m_efd.get(), POLLIN, 0}}; int ret = poll(&pfd[0], 2, -1); @@ -165,7 +165,7 @@ StatsServer::threadFn() if (pfd[0].revents & POLLIN) { socklen_t addrlen{sizeof(addr)}; - int conn{accept(sock, reinterpret_cast(&addr), &addrlen)}; + int conn{accept(sock.get(), reinterpret_cast(&addr), &addrlen)}; if (conn == -1) { continue; } @@ -176,11 +176,6 @@ StatsServer::threadFn() break; } } - -out: - if (sock != -1) { - close(sock); - } } StatsClient::StatsClient(pid_t p) diff --git a/test/amd_detail/stats.cpp b/test/amd_detail/stats.cpp index 356cbdb8..ed943098 100644 --- a/test/amd_detail/stats.cpp +++ b/test/amd_detail/stats.cpp @@ -55,7 +55,7 @@ TEST_F(HipFileStats, StatsServerLifetime) EXPECT_CALL(msys, ftruncate); EXPECT_CALL(msys, mmap).WillOnce(testing::Return(&buff)); EXPECT_CALL(msys, munmap); - EXPECT_CALL(msys, close).Times(2); + EXPECT_CALL(msys, close).Times(3); EXPECT_CALL(mcfg, statsLevel()).WillOnce(testing::Return(1)); StatsServer srvr{}; }