Use weak_ptr for TrapHandler Callbacks

The lifetime of the `this` pointer in the trap callbacks could be invalid as the lifetime of the underlying `Buffer`/`Texture` object wasn't guaranteed, this commit fixes that by passing a `weak_ptr` of the objects into the callbacks which is locked during the callbacks and ensures that a destroyed object isn't accessed.

Co-authored-by: Billy Laws <blaws05@gmail.com>

app/src/main/cpp/skyline/gpu/buffer.cpp

@@ -15,23 +15,37 @@ namespace skyline::gpu {
         alignedMirror = gpu.state.process->memory.CreateMirror(span<u8>{alignedData, alignedSize});
         mirror = alignedMirror.subspan(static_cast<size_t>(guest->data() - alignedData), guest->size());
 
-        trapHandle = gpu.state.nce->TrapRegions(*guest, true, [this] {
+        // We can't just capture `this` in the lambda since the lambda could exceed the lifetime of the buffer
-            std::scoped_lock lock{*this};
+        std::weak_ptr<Buffer> weakThis{weak_from_this()};
-        }, [this] {
+        trapHandle = gpu.state.nce->TrapRegions(*guest, true, [weakThis] {
-            std::unique_lock lock{*this, std::try_to_lock};
+            auto buffer{weakThis.lock()};
+            if (!buffer)
+                return;
+            std::scoped_lock lock{*buffer};
+        }, [weakThis] {
+            auto buffer{weakThis.lock()};
+            if (!buffer)
+                return true;
+
+            std::unique_lock lock{*buffer, std::try_to_lock};
             if (!lock)
                 return false;
             SynchronizeGuest(true); // We can skip trapping since the caller will do it
+
             return true;
-        }, [this] {
+        }, [weakThis] {
+            auto buffer{weakThis.lock()};
+            if (!buffer)
+                return true;
+
             DirtyState expectedState{DirtyState::Clean};
-            if (dirtyState.compare_exchange_strong(expectedState, DirtyState::CpuDirty, std::memory_order_relaxed) || expectedState == DirtyState::CpuDirty)
+            if (buffer->dirtyState.compare_exchange_strong(expectedState, DirtyState::CpuDirty, std::memory_order_relaxed) || expectedState == DirtyState::CpuDirty)
                 return true; // If we can transition the buffer to CPU dirty (from Clean) or if it already is CPU dirty then we can just return, we only need to do the lock and corresponding sync if the buffer is GPU dirty
 
-            std::unique_lock lock{*this, std::try_to_lock};
+            std::unique_lock lock{*buffer, std::try_to_lock};
             if (!lock)
                 return false;
-            SynchronizeGuest(true, false, true); // We need to assume the buffer is dirty since we don't know what the guest is writing
+            buffer->SynchronizeGuest(true, false, true); // We need to assume the buffer is dirty since we don't know what the guest is writing
             return true;
         });
     }

app/src/main/cpp/skyline/gpu/buffer.h

@@ -117,7 +117,7 @@ namespace skyline::gpu {
         friend BufferManager;
 
         /**
-         * @brief Sets up mirror mappings for the guest mappings
+         * @brief Sets up mirror mappings for the guest mappings, this must be called after construction for the mirror to be valid
          */
         void SetupGuestMappings();
 
@@ -138,6 +138,10 @@ namespace skyline::gpu {
             return span<u8>(backing);
         }
 
+        /**
+         * @brief Creates a buffer object wrapping the guest buffer with a backing that can represent the guest buffer data
+         * @note The guest mappings will not be setup until SetupGuestMappings() is called
+         */
         Buffer(GPU &gpu, GuestBuffer guest);
 
         /**

app/src/main/cpp/skyline/gpu/buffer_manager.cpp

@@ -69,6 +69,7 @@ namespace skyline::gpu {
 
         LockedBuffer newBuffer{std::make_shared<Buffer>(gpu, span<u8>{lowestAddress, highestAddress}), tag}; // If we don't lock the buffer prior to trapping it during synchronization, a race could occur with a guest trap acquiring the lock before we do and mutating the buffer prior to it being ready
 
+        newBuffer->SetupGuestMappings();
         newBuffer->SynchronizeHost(false); // Overlaps don't necessarily fully cover the buffer so we have to perform a sync here to prevent any gaps
 
         auto copyBuffer{[](auto dstGuest, auto srcGuest, auto dstPtr, auto srcPtr) {
@@ -164,6 +165,7 @@ namespace skyline::gpu {
         if (overlaps.empty()) {
             // If we couldn't find any overlapping buffers, create a new buffer without coalescing
             LockedBuffer buffer{std::make_shared<Buffer>(gpu, guestMapping), tag};
+            buffer->SetupGuestMappings();
             buffer->SynchronizeHost();
             InsertBuffer(*buffer);
             return buffer->GetView(offset, size);

app/src/main/cpp/skyline/gpu/texture/texture.cpp

@@ -143,25 +143,40 @@ namespace skyline::gpu {
             mirror = alignedMirror.subspan(static_cast<size_t>(frontMapping.data() - alignedData), totalSize);
         }
 
-        trapHandle = gpu.state.nce->TrapRegions(mappings, true, [this] {
+        // We can't just capture `this` in the lambda since the lambda could exceed the lifetime of the buffer
-            std::scoped_lock lock{*this};
+        std::weak_ptr<Texture> weakThis{weak_from_this()};
-        }, [this] {
+        trapHandle = gpu.state.nce->TrapRegions(mappings, true, [weakThis] {
-            std::unique_lock lock{*this, std::try_to_lock};
+            auto texture{weakThis.lock()};
+            if (!texture)
+                return;
+
+            std::scoped_lock lock{*texture};
+        }, [weakThis] {
+            auto texture{weakThis.lock()};
+            if (!texture)
+                return true;
+
+            std::unique_lock lock{*texture, std::try_to_lock};
             if (!lock)
                 return false;
-            SynchronizeGuest(true); // We can skip trapping since the caller will do it
+
-            WaitOnFence();
+            texture->SynchronizeGuest(true); // We can skip trapping since the caller will do it
+            texture->WaitOnFence();
             return true;
-        }, [this] {
+        }, [weakThis] {
+            auto texture{weakThis.lock()};
+            if (!texture)
+                return true;
+
             DirtyState expectedState{DirtyState::Clean};
-            if (dirtyState.compare_exchange_strong(expectedState, DirtyState::CpuDirty, std::memory_order_relaxed) || expectedState == DirtyState::CpuDirty)
+            if (texture->dirtyState.compare_exchange_strong(expectedState, DirtyState::CpuDirty, std::memory_order_relaxed) || expectedState == DirtyState::CpuDirty)
                 return true; // If we can transition the texture to CPU dirty (from Clean) or if it already is CPU dirty then we can just return, we only need to do the lock and corresponding sync if the texture is GPU dirty
 
-            std::unique_lock lock{*this, std::try_to_lock};
+            std::unique_lock lock{*texture, std::try_to_lock};
             if (!lock)
                 return false;
-            SynchronizeGuest(true, true); // We need to assume the texture is dirty since we don't know what the guest is writing
+            texture->SynchronizeGuest(true, true); // We need to assume the texture is dirty since we don't know what the guest is writing
-            WaitOnFence();
+            texture->WaitOnFence();
             return true;
         });
     }

app/src/main/cpp/skyline/gpu/texture/texture.h

@@ -390,7 +390,7 @@ namespace skyline::gpu {
         friend TextureView;
 
         /**
-         * @brief Sets up mirror mappings for the guest mappings
+         * @brief Sets up mirror mappings for the guest mappings, this must be called after construction for the mirror to be valid
          */
         void SetupGuestMappings();
 
@@ -460,6 +460,7 @@ namespace skyline::gpu {
 
         /**
          * @brief Creates a texture object wrapping the guest texture with a backing that can represent the guest texture data
+         * @note The guest mappings will not be setup until SetupGuestMappings() is called
          */
         Texture(GPU &gpu, GuestTexture guest);
 

app/src/main/cpp/skyline/gpu/texture_manager.cpp

@@ -87,6 +87,7 @@ namespace skyline::gpu {
 
         // Create a texture as we cannot find one that matches
         auto texture{std::make_shared<Texture>(gpu, guestTexture)};
+        texture->SetupGuestMappings();
         texture->TransitionLayout(vk::ImageLayout::eGeneral);
         auto it{texture->guest->mappings.begin()};
         textures.emplace(mappingEnd, TextureMapping{texture, it, guestMapping});