Merge pull request #21669 from hrydgard/vulkan-buffer-overflow-fix

Add "allocation slack" to our pushbuffers. Fixes a memory overwrite bug

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);
 }

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_;
 };

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;
 	}

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)) {

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++) {

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<Block> blocks_;
 	VkBufferUsageFlags usage_;
 	int curBlockIndex_ = -1;
+	VkDeviceSize slack_;
 	const char *name_;
 };
 

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

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;
 	}
 

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

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);

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;

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"

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;