From f1fabcfd64733a20db5719735ea4e79648a81960 Mon Sep 17 00:00:00 2001 From: "systems-assistant[bot]" <221163467+systems-assistant[bot]@users.noreply.github.com> Date: Tue, 16 Sep 2025 09:43:45 -0400 Subject: [PATCH] rocr: Error Handling Issues (#264) * rocr: Fix Incorrect Assertion Check The wrong variable is used in the assertion statement, should be error checking for the value of paramEndLoc after it is modified by the call to find(). Signed-off-by: Sunday Clement * rocr: Fix Potential Undefined Behaviour In the event that the SvmProfileControl destructor is called and event == -1 is true then the call to close(event) is effectively close(-1) which is undefined behaviour. This has been changed to only call close() on valid file descriptors. Signed-off-by: Sunday Clement * rocr: Add Error Check on Bytes Read In the case that there is an incomplete read the call to copyTo() will now return an error. Signed-off-by: Sunday Clement * rocr: Fix Exception Error Destructors are implicitly marked with noexcept being true by default so if its not explicitly marked false in the destructor or the functions it calls, any thrown exceptions will cause the program to crash. Signed-off-by: Sunday Clement --------- Signed-off-by: Sunday Clement Co-authored-by: Sunday Clement --- .../hsa-runtime/core/runtime/amd_blit_kernel.cpp | 2 +- .../hsa-runtime/core/runtime/svm_profiler.cpp | 6 ++++-- .../runtime/hsa-runtime/core/util/utils.h | 2 +- .../hsa-runtime/libamdhsacode/amd_elf_image.cpp | 16 ++++++++++++++-- 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_blit_kernel.cpp b/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_blit_kernel.cpp index 64f6599b8d..8cdb0603cf 100644 --- a/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_blit_kernel.cpp +++ b/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/amd_blit_kernel.cpp @@ -506,7 +506,7 @@ int GetKernelSourceParam(const char* paramName) { std::string::size_type paramValLoc = paramDefLoc + paramDef.str().size(); std::string::size_type paramEndLoc = kBlitKernelSource().find('\n', paramDefLoc); - assert(paramDefLoc != std::string::npos); + assert(paramEndLoc != std::string::npos); std::string paramVal(&kBlitKernelSource()[paramValLoc], &kBlitKernelSource()[paramEndLoc]); diff --git a/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/svm_profiler.cpp b/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/svm_profiler.cpp index b63d89ee4f..8d09c744b5 100644 --- a/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/svm_profiler.cpp +++ b/projects/rocr-runtime/runtime/hsa-runtime/core/runtime/svm_profiler.cpp @@ -356,14 +356,16 @@ SvmProfileControl::SvmProfileControl() : event(-1), exit(false) { } SvmProfileControl::~SvmProfileControl() { - if (event != -1) eventfd_write(event, 1); + if (event != -1) { + eventfd_write(event, 1); + close(event); + } if (poll_smi_thread_ != NULL) { exit = true; os::WaitForThread(poll_smi_thread_); os::CloseThread(poll_smi_thread_); poll_smi_thread_ = NULL; } - close(event); } template diff --git a/projects/rocr-runtime/runtime/hsa-runtime/core/util/utils.h b/projects/rocr-runtime/runtime/hsa-runtime/core/util/utils.h index 18c0166b9e..c2d57df4fa 100644 --- a/projects/rocr-runtime/runtime/hsa-runtime/core/util/utils.h +++ b/projects/rocr-runtime/runtime/hsa-runtime/core/util/utils.h @@ -179,7 +179,7 @@ class ScopeGuard { ScopeGuard(ScopeGuard& rhs) { *this = rhs; } - __forceinline ~ScopeGuard() { + __forceinline ~ScopeGuard() noexcept(false) { if (!dismiss_) release_(); } __forceinline ScopeGuard& operator=(ScopeGuard& rhs) { diff --git a/projects/rocr-runtime/runtime/hsa-runtime/libamdhsacode/amd_elf_image.cpp b/projects/rocr-runtime/runtime/hsa-runtime/libamdhsacode/amd_elf_image.cpp index 2e63ea35e8..1fa7e1de13 100644 --- a/projects/rocr-runtime/runtime/hsa-runtime/libamdhsacode/amd_elf_image.cpp +++ b/projects/rocr-runtime/runtime/hsa-runtime/libamdhsacode/amd_elf_image.cpp @@ -274,7 +274,16 @@ namespace elf { { size_t size1 = getSize(); void* buffer1 = malloc(size1); - if (_read(d, buffer1, size1) < 0) { free(buffer1); return perror("read failed"); } + ssize_t bytes_read = _read(d, buffer1, size1); + if (bytes_read < 0) { + free(buffer1); + return perror("read failed"); + } + if (static_cast(bytes_read) != size1) { + free(buffer1); + return perror("Incomplete read"); + } + *buffer = buffer1; if (size) { *size = size1; } return true; @@ -284,7 +293,10 @@ namespace elf { { size_t size1 = getSize(); if (size < size1) { return error("Buffer size is not enough"); } - if (_read(d, buffer, size1) < 0) { return perror("read failed"); } + ssize_t bytes_read = _read(d, buffer, size1); + if (bytes_read < 0) { return perror("read failed"); } + if (static_cast(bytes_read) != size1) { return perror("Incomplete read"); } + return true; }