From 28166cb35ddba5ba924d1e16e978b475bdcdce78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 13 May 2026 15:55:34 +0200 Subject: [PATCH] Add "allocation slack" to our pushbuffers. Fixes a memory overwrite bug Reported by Joseph on Discord. Sometimes, things could align perfectly so allocations happend exactly at the end of a pushbuffer. At the same time, we allow our vertex decoder to write an extra few bytes if it needs to for speed. Unfortunately I missed this interaction, resulting in some uncommon crashes that were especially common with heavy-geometry things like modified GTA LCS with PS2 assets, for example. The problem was reported with Vulkan, but our OpenGL backend had the same issue too. --- Common/GPU/OpenGL/GLMemory.cpp | 3 ++- Common/GPU/OpenGL/GLMemory.h | 10 +++++++--- Common/GPU/OpenGL/GLRenderManager.h | 4 ++-- Common/GPU/OpenGL/thin3d_gl.cpp | 2 +- Common/GPU/Vulkan/VulkanMemory.cpp | 4 ++-- Common/GPU/Vulkan/VulkanMemory.h | 9 ++++++--- Common/GPU/Vulkan/thin3d_vulkan.cpp | 2 +- GPU/Common/TextureReplacer.cpp | 2 +- GPU/Common/VertexDecoderCommon.h | 3 +++ GPU/GLES/DrawEngineGLES.cpp | 4 ++-- GPU/Vulkan/DrawEngineVulkan.cpp | 4 ++-- GPU/Vulkan/ShaderManagerVulkan.cpp | 2 ++ GPU/Vulkan/TextureCacheVulkan.h | 1 - 13 files changed, 31 insertions(+), 19 deletions(-) diff --git a/Common/GPU/OpenGL/GLMemory.cpp b/Common/GPU/OpenGL/GLMemory.cpp index 5bbb40e9b5..86fb57ab76 100644 --- a/Common/GPU/OpenGL/GLMemory.cpp +++ b/Common/GPU/OpenGL/GLMemory.cpp @@ -62,7 +62,8 @@ bool GLRBuffer::Unmap() { return glUnmapBuffer(target_) == GL_TRUE; } -GLPushBuffer::GLPushBuffer(GLRenderManager *render, GLuint target, size_t size, const char *tag) : render_(render), nextBufferSize_(size), target_(target), tag_(tag) { +GLPushBuffer::GLPushBuffer(GLRenderManager *render, GLuint target, size_t size, int slack, const char *tag) + : render_(render), nextBufferSize_(size), target_(target), slack_(slack), tag_(tag) { AddBuffer(); RegisterGPUMemoryManager(this); } diff --git a/Common/GPU/OpenGL/GLMemory.h b/Common/GPU/OpenGL/GLMemory.h index 4aa86217f5..3d7780f621 100644 --- a/Common/GPU/OpenGL/GLMemory.h +++ b/Common/GPU/OpenGL/GLMemory.h @@ -74,7 +74,9 @@ public: size_t size; }; - GLPushBuffer(GLRenderManager *render, GLuint target, size_t size, const char *tag); + // Slack is reserved space at the end of each block, which can be useful if you do things like writing a vec3 with a vec4 store, + // which can be faster when using SIMD. It probably never has to be larger than 32 bytes. + GLPushBuffer(GLRenderManager *render, GLuint target, size_t size, int slack, const char *tag); ~GLPushBuffer(); void Reset() { offset_ = 0; } @@ -116,7 +118,7 @@ public: // again, call Rewind (see below). uint8_t *Allocate(uint32_t numBytes, uint32_t alignment, GLRBuffer **buf, uint32_t *bindOffset) { uint32_t offset = ((uint32_t)offset_ + alignment - 1) & ~(alignment - 1); - if (offset + numBytes <= nextBufferSize_) { + if (offset + numBytes + slack_ <= nextBufferSize_) { // Common path. offset_ = offset + numBytes; *buf = buffers_[buf_].buffer; @@ -132,7 +134,8 @@ public: return writePtr_; } - // For convenience if all you'll do is to copy. + // NOTE: If you can avoid this by writing the data directly into memory returned from Allocate, + // do so. Savings from avoiding memcpy can be significant. uint32_t Push(const void *data, uint32_t numBytes, int alignment, GLRBuffer **buf) { uint32_t bindOffset; uint8_t *ptr = Allocate(numBytes, alignment, buf, &bindOffset); @@ -179,5 +182,6 @@ private: uint8_t *writePtr_ = nullptr; GLuint target_; GLBufferStrategy strategy_ = GLBufferStrategy::SUBDATA; + int slack_; const char *tag_; }; diff --git a/Common/GPU/OpenGL/GLRenderManager.h b/Common/GPU/OpenGL/GLRenderManager.h index ca3d038581..6c97169634 100644 --- a/Common/GPU/OpenGL/GLRenderManager.h +++ b/Common/GPU/OpenGL/GLRenderManager.h @@ -341,8 +341,8 @@ public: return step.create_input_layout.inputLayout; } - GLPushBuffer *CreatePushBuffer(int frame, GLuint target, size_t size, const char *tag) { - GLPushBuffer *push = new GLPushBuffer(this, target, size, tag); + GLPushBuffer *CreatePushBuffer(int frame, GLuint target, size_t size, int slack, const char *tag) { + GLPushBuffer *push = new GLPushBuffer(this, target, size, slack, tag); RegisterPushBuffer(frame, push); return push; } diff --git a/Common/GPU/OpenGL/thin3d_gl.cpp b/Common/GPU/OpenGL/thin3d_gl.cpp index 57279e0c3e..046ab99f64 100644 --- a/Common/GPU/OpenGL/thin3d_gl.cpp +++ b/Common/GPU/OpenGL/thin3d_gl.cpp @@ -644,7 +644,7 @@ OpenGLContext::OpenGLContext(bool canChangeSwapInterval) : renderManager_(frameT caps_.isTilingGPU = gl_extensions.IsGLES && caps_.vendor != GPUVendor::VENDOR_NVIDIA && caps_.vendor != GPUVendor::VENDOR_INTEL; for (int i = 0; i < GLRenderManager::MAX_INFLIGHT_FRAMES; i++) { - frameData_[i].push = renderManager_.CreatePushBuffer(i, GL_ARRAY_BUFFER, 64 * 1024, "thin3d_vbuf"); + frameData_[i].push = renderManager_.CreatePushBuffer(i, GL_ARRAY_BUFFER, 64 * 1024, 32, "thin3d_vbuf"); } if (!gl_extensions.VersionGEThan(3, 0, 0)) { diff --git a/Common/GPU/Vulkan/VulkanMemory.cpp b/Common/GPU/Vulkan/VulkanMemory.cpp index e54f385b7a..c853a5aa10 100644 --- a/Common/GPU/Vulkan/VulkanMemory.cpp +++ b/Common/GPU/Vulkan/VulkanMemory.cpp @@ -31,8 +31,8 @@ using namespace PPSSPP_VK; // Always keep around push buffers at least this long (seconds). static const double PUSH_GARBAGE_COLLECTION_DELAY = 10.0; -VulkanPushPool::VulkanPushPool(VulkanContext *vulkan, const char *name, size_t originalBlockSize, VkBufferUsageFlags usage) - : vulkan_(vulkan), name_(name), originalBlockSize_(originalBlockSize), usage_(usage) { +VulkanPushPool::VulkanPushPool(VulkanContext *vulkan, const char *name, size_t originalBlockSize, size_t slack, VkBufferUsageFlags usage) + : vulkan_(vulkan), name_(name), originalBlockSize_(originalBlockSize), usage_(usage), slack_(slack) { RegisterGPUMemoryManager(this); for (int i = 0; i < VulkanContext::MAX_INFLIGHT_FRAMES; i++) { diff --git a/Common/GPU/Vulkan/VulkanMemory.h b/Common/GPU/Vulkan/VulkanMemory.h index fe0bc576d5..1313d21813 100644 --- a/Common/GPU/Vulkan/VulkanMemory.h +++ b/Common/GPU/Vulkan/VulkanMemory.h @@ -21,7 +21,9 @@ VK_DEFINE_HANDLE(VmaAllocation); // NOT thread safe! Can only be used from one thread (our main thread). class VulkanPushPool : public GPUMemoryManager { public: - VulkanPushPool(VulkanContext *vulkan, const char *name, size_t originalBlockSize, VkBufferUsageFlags usage); + // Slack is reserved space at the end of each block, which can be useful if you do things like writing a vec3 with a vec4 store, + // which can be faster when using SIMD, or decode two vertices in parallel. + VulkanPushPool(VulkanContext *vulkan, const char *name, size_t originalBlockSize, size_t slack, VkBufferUsageFlags usage); ~VulkanPushPool(); void Destroy(); @@ -40,7 +42,7 @@ public: Block &block = blocks_[curBlockIndex_]; VkDeviceSize offset = (block.used + (alignment - 1)) & ~(alignment - 1); - if (offset + numBytes <= block.size) { + if (offset + numBytes + slack_ <= block.size) { block.used = offset + numBytes; *vkbuf = block.buffer; *bindOffset = (uint32_t)offset; @@ -76,7 +78,7 @@ private: VkDeviceSize size; VkDeviceSize used; - int frameIndex; + int frameIndex; // -1 means that it's "common", it can be grabbed by any frame as needed. bool original; // these blocks aren't garbage collected. double lastUsed; @@ -92,6 +94,7 @@ private: std::vector blocks_; VkBufferUsageFlags usage_; int curBlockIndex_ = -1; + VkDeviceSize slack_; const char *name_; }; diff --git a/Common/GPU/Vulkan/thin3d_vulkan.cpp b/Common/GPU/Vulkan/thin3d_vulkan.cpp index bcf4ff350c..d0f2fd37f4 100644 --- a/Common/GPU/Vulkan/thin3d_vulkan.cpp +++ b/Common/GPU/Vulkan/thin3d_vulkan.cpp @@ -1127,7 +1127,7 @@ VKContext::VKContext(VulkanContext *vulkan, bool useRenderThread) device_ = vulkan->GetDevice(); VkBufferUsageFlags usage = VK_BUFFER_USAGE_INDEX_BUFFER_BIT | VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT | VK_BUFFER_USAGE_STORAGE_BUFFER_BIT | VK_BUFFER_USAGE_VERTEX_BUFFER_BIT | VK_BUFFER_USAGE_TRANSFER_SRC_BIT; - push_ = new VulkanPushPool(vulkan_, "pushBuffer", 4 * 1024 * 1024, usage); + push_ = new VulkanPushPool(vulkan_, "pushBuffer", 4 * 1024 * 1024, 32, usage); // binding 0 - uniform data // binding 1 - combined sampler/image 0 diff --git a/GPU/Common/TextureReplacer.cpp b/GPU/Common/TextureReplacer.cpp index 660f00678b..e0add5a204 100644 --- a/GPU/Common/TextureReplacer.cpp +++ b/GPU/Common/TextureReplacer.cpp @@ -496,7 +496,7 @@ void TextureReplacer::ParseReduceHashRange(const std::string& key, const std::st } if (rhashvalue == 0) { - ERROR_LOG(Log::TexReplacement, "Ignoring invalid hashrange %s = %s, reducehashvalue can't be 0", key.c_str(), value.c_str()); + ERROR_LOG(Log::TexReplacement, "Ignoring invalid reducehashrange %s = %s, reducehashvalue can't be 0", key.c_str(), value.c_str()); return; } diff --git a/GPU/Common/VertexDecoderCommon.h b/GPU/Common/VertexDecoderCommon.h index 1a8dcb8535..696eee018d 100644 --- a/GPU/Common/VertexDecoderCommon.h +++ b/GPU/Common/VertexDecoderCommon.h @@ -366,6 +366,9 @@ public: const DecVtxFormat &GetDecVtxFmt() const { return decFmt; } + // WARNING: This may write up to a full extra vertex plus 16 bytes (in practice less, but let's define it that way to be future proof) extra bytes after + // the end of the buffer, so make sure you have some extra space there (that you can safely overwrite after Decode). + // In VulkanPushBuffer / GLPushBuffer, use the slack parameter. Why not 256, that should cover every case. void DecodeVerts(u8 *decoded, const u8 *startPtr, const UVScale *uvScaleOffset, int count) const; int VertexSize() const { return size; } // PSP format size diff --git a/GPU/GLES/DrawEngineGLES.cpp b/GPU/GLES/DrawEngineGLES.cpp index 133a77fdf4..7a5c760f9b 100644 --- a/GPU/GLES/DrawEngineGLES.cpp +++ b/GPU/GLES/DrawEngineGLES.cpp @@ -84,8 +84,8 @@ void DrawEngineGLES::InitDeviceObjects() { _assert_msg_(render_ != nullptr, "Render manager must be set"); for (int i = 0; i < GLRenderManager::MAX_INFLIGHT_FRAMES; i++) { - frameData_[i].pushVertex = render_->CreatePushBuffer(i, GL_ARRAY_BUFFER, 2048 * 1024, "game_vertex"); - frameData_[i].pushIndex = render_->CreatePushBuffer(i, GL_ELEMENT_ARRAY_BUFFER, 256 * 1024, "game_index"); + frameData_[i].pushVertex = render_->CreatePushBuffer(i, GL_ARRAY_BUFFER, 2 * 1024 * 1024, 256, "game_vertex"); + frameData_[i].pushIndex = render_->CreatePushBuffer(i, GL_ELEMENT_ARRAY_BUFFER, 256 * 1024, 64, "game_index"); } int stride = sizeof(TransformedVertex); diff --git a/GPU/Vulkan/DrawEngineVulkan.cpp b/GPU/Vulkan/DrawEngineVulkan.cpp index c9d8e6292a..7ba0c615a5 100644 --- a/GPU/Vulkan/DrawEngineVulkan.cpp +++ b/GPU/Vulkan/DrawEngineVulkan.cpp @@ -76,8 +76,8 @@ void DrawEngineVulkan::InitDeviceObjects() { pipelineLayout_ = renderManager->CreatePipelineLayout(bindingTypes, ARRAY_SIZE(bindingTypes), draw_->GetDeviceCaps().geometryShaderSupported, "drawengine_layout"); pushUBO_ = (VulkanPushPool *)draw_->GetNativeObject(Draw::NativeObject::PUSH_POOL); - pushVertex_ = new VulkanPushPool(vulkan, "pushVertex", 4 * 1024 * 1024, VK_BUFFER_USAGE_VERTEX_BUFFER_BIT); - pushIndex_ = new VulkanPushPool(vulkan, "pushIndex", 1 * 512 * 1024, VK_BUFFER_USAGE_INDEX_BUFFER_BIT); + pushVertex_ = new VulkanPushPool(vulkan, "pushVertex", 4 * 1024 * 1024, 256, VK_BUFFER_USAGE_VERTEX_BUFFER_BIT); + pushIndex_ = new VulkanPushPool(vulkan, "pushIndex", 512 * 1024, 64, VK_BUFFER_USAGE_INDEX_BUFFER_BIT); VkSamplerCreateInfo samp{ VK_STRUCTURE_TYPE_SAMPLER_CREATE_INFO }; samp.addressModeU = VK_SAMPLER_ADDRESS_MODE_CLAMP_TO_EDGE; diff --git a/GPU/Vulkan/ShaderManagerVulkan.cpp b/GPU/Vulkan/ShaderManagerVulkan.cpp index 3c38d8d05c..9de353532c 100644 --- a/GPU/Vulkan/ShaderManagerVulkan.cpp +++ b/GPU/Vulkan/ShaderManagerVulkan.cpp @@ -28,6 +28,8 @@ #include "Common/GPU/Vulkan/VulkanContext.h" #include "Common/Log.h" #include "Common/TimeUtil.h" +#include "Common/GPU/Vulkan/VulkanMemory.h" + #include "GPU/GPUState.h" #include "GPU/Common/FragmentShaderGenerator.h" #include "GPU/Common/VertexShaderGenerator.h" diff --git a/GPU/Vulkan/TextureCacheVulkan.h b/GPU/Vulkan/TextureCacheVulkan.h index 520567eace..9da40d692d 100644 --- a/GPU/Vulkan/TextureCacheVulkan.h +++ b/GPU/Vulkan/TextureCacheVulkan.h @@ -23,7 +23,6 @@ #include "GPU/Common/TextureCacheCommon.h" #include "GPU/Common/TextureShaderCommon.h" #include "GPU/Vulkan/VulkanUtil.h" -#include "GPU/Vulkan/VulkanMemory.h" struct VirtualFramebuffer; struct TextureShaderInfo;