Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

race condition in native cpu adaptor allocation functions #2070

Open
ldrumm opened this issue Sep 9, 2024 · 1 comment
Open

race condition in native cpu adaptor allocation functions #2070

ldrumm opened this issue Sep 9, 2024 · 1 comment

Comments

@ldrumm
Copy link
Contributor

ldrumm commented Sep 9, 2024

There's a subtle race condition in get_alloc_info_entry.
get_alloc_info_entry protects the set of known allocations by grabbing the mutex protecting the same, but then returns a reference to the allocation by which point the mutex has been released.

It's plausible that by the time the caller dereferences the returned reference another thread may have freed the given allocation.

At first I thought that this might be enough:

diff --git a/source/adapters/native_cpu/context.hpp b/source/adapters/native_cpu/context.hpp
index c59ab4ea..d3f54b3d 100644
--- a/source/adapters/native_cpu/context.hpp
+++ b/source/adapters/native_cpu/context.hpp
@@ -110,7 +110,7 @@ struct ur_context_handle_t_ : RefCounted {
   }

   // Note this is made non-const to access the mutex
-  const native_cpu::usm_alloc_info &get_alloc_info_entry(const void *ptr) {
+  const native_cpu::usm_alloc_info get_alloc_info_entry(const void *ptr) {
     std::lock_guard<std::mutex> lock(alloc_mutex);
     auto it = allocations.find(ptr);
     if (it == allocations.end()) {

However, returning a value type here doesn't actually help since although it'll prevent a segfault or similar UB, there's no way to determine whether the information returned is still valid.

I'm not sure whether it's something we see in the wild, but it's a real possibility so might be worth a different approach

@ldrumm
Copy link
Contributor Author

ldrumm commented Sep 9, 2024

Maybe this is a user-is-responsible problem and we don't care?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant