[SYCL] Fix the check for read-only host pointer during memobj creation (#2697)

Previously, whenever a native memory object was created, the original host
pointer passed by the user was checked for being read-only, regardless
of what host pointer was actually used at that point. This patch fixes the
issue by assuming a writeable host pointer unless it's the one provided by
the user.

As a result, this fixes a double free issue that appeared with the
following usage pattern:
  1. Create a buffer with a const user pointer
  2. Write-access it on host
  3. Access it on a non-host device
  4. Access it on host

This error was caused by the allocation performed at step 2 being
treated as read-only and, therefore, used with
PI_MEM_FLAGS_HOST_PTR_COPY rather than PI_MEM_FLAGS_HOST_PTR_USE at step
3, while it was assumed otherwise during the release of this allocation.

Signed-off-by: Sergey Semenov <[email protected]>

Author: sergey-semenov

sycl/source/detail/buffer_impl.cpp

@@ -16,21 +16,26 @@ namespace sycl {
 namespace detail {
 void *buffer_impl::allocateMem(ContextImplPtr Context, bool InitFromUserData,
                   void *HostPtr, RT::PiEvent &OutEventToWait) {
+  // The host pointer for the allocation can be provided in 2 ways:
+  // 1. Initialize the allocation from user data. Check if the user pointer is
+  // read-only.
+  // 2. Use a HostPtr allocated by the runtime. Assume any such pointer to be
+  // read-write.
+  bool HostPtrReadOnly = false;
+  if (InitFromUserData) {
+    assert(!HostPtr && "Cannot init from user data and reuse host ptr provided "
+                       "simultaneously");
+    HostPtr = BaseT::getUserPtr();
+    HostPtrReadOnly = BaseT::MHostPtrReadOnly;
+  }
 
-  assert(!(InitFromUserData && HostPtr) &&
-          "Cannot init from user data and reuse host ptr provided "
-          "simultaneously");
-
-  void *UserPtr = InitFromUserData ? BaseT::getUserPtr() : HostPtr;
-
-  assert(!(nullptr == UserPtr && BaseT::useHostPtr() && Context->is_host()) &&
-          "Internal error. Allocating memory on the host "
-          "while having use_host_ptr property");
+  assert(!(nullptr == HostPtr && BaseT::useHostPtr() && Context->is_host()) &&
+         "Internal error. Allocating memory on the host "
+         "while having use_host_ptr property");
 
   return MemoryManager::allocateMemBuffer(
-      std::move(Context), this, UserPtr, BaseT::MHostPtrReadOnly,
-      BaseT::getSize(), BaseT::MInteropEvent, BaseT::MInteropContext, MProps,
-      OutEventToWait);
+      std::move(Context), this, HostPtr, HostPtrReadOnly, BaseT::getSize(),
+      BaseT::MInteropEvent, BaseT::MInteropContext, MProps, OutEventToWait);
 }
 } // namespace detail
 } // namespace sycl

sycl/source/detail/image_impl.cpp

@@ -307,25 +307,31 @@ template <int Dimensions>
 void *image_impl<Dimensions>::allocateMem(ContextImplPtr Context,
                                           bool InitFromUserData, void *HostPtr,
                                           RT::PiEvent &OutEventToWait) {
+  // The host pointer for the allocation can be provided in 2 ways:
+  // 1. Initialize the allocation from user data. Check if the user pointer is
+  // read-only.
+  // 2. Use a HostPtr allocated by the runtime. Assume any such pointer to be
+  // read-write.
+  bool HostPtrReadOnly = false;
+  if (InitFromUserData) {
+    assert(!HostPtr && "Cannot init from user data and reuse host ptr provided "
+                       "simultaneously");
+    HostPtr = BaseT::getUserPtr();
+    HostPtrReadOnly = BaseT::MHostPtrReadOnly;
+  }
 
-  assert(!(InitFromUserData && HostPtr) &&
-         "Cannot init from user data and reuse host ptr provided "
-         "simultaneously");
-
-  void *UserPtr = InitFromUserData ? BaseT::getUserPtr() : HostPtr;
-
-  RT::PiMemImageDesc Desc = getImageDesc(UserPtr != nullptr);
-  assert(checkImageDesc(Desc, Context, UserPtr) &&
+  RT::PiMemImageDesc Desc = getImageDesc(HostPtr != nullptr);
+  assert(checkImageDesc(Desc, Context, HostPtr) &&
          "The check an image desc failed.");
 
   RT::PiMemImageFormat Format = getImageFormat();
   assert(checkImageFormat(Format, Context) &&
          "The check an image format failed.");
 
   return MemoryManager::allocateMemImage(
-      std::move(Context), this, UserPtr, BaseT::MHostPtrReadOnly,
-      BaseT::getSize(), Desc, Format, BaseT::MInteropEvent,
-      BaseT::MInteropContext, MProps, OutEventToWait);
+      std::move(Context), this, HostPtr, HostPtrReadOnly, BaseT::getSize(),
+      Desc, Format, BaseT::MInteropEvent, BaseT::MInteropContext, MProps,
+      OutEventToWait);
 }
 
 template <int Dimensions>

sycl/test/basic_tests/buffer/native_buffer_creation_flags.cpp

@@ -0,0 +1,28 @@
+// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out
+// RUN: %CPU_RUN_PLACEHOLDER SYCL_PI_TRACE=-1 %t.out 2>&1 %CPU_CHECK_PLACEHOLDER
+
+#include <CL/sycl.hpp>
+
+class Foo;
+using namespace cl::sycl;
+int main() {
+  const int BufVal = 42;
+  buffer<int, 1> Buf{&BufVal, range<1>(1)};
+  queue Q;
+
+  {
+    // This should trigger memory allocation on host since the pointer passed by
+    // the user is read-only.
+    auto BufAcc = Buf.get_access<access::mode::write>();
+  }
+
+  Q.submit([&](handler &Cgh) {
+    // Now that we have a read-write host allocation, check that the native
+    // buffer is created with the PI_MEM_FLAGS_HOST_PTR_USE flag.
+    // CHECK: piMemBufferCreate
+    // CHECK-NEXT: {{.*}} : {{.*}}
+    // CHECK-NEXT: {{.*}} : 9
+    auto BufAcc = Buf.get_access<access::mode::read>(Cgh);
+    Cgh.single_task<Foo>([=]() { int A = BufAcc[0]; });
+  });
+}