From 4f02905510f873c5e9b25b497be58819029b0429 Mon Sep 17 00:00:00 2001 From: Ion Agorria Date: Thu, 22 Feb 2024 00:48:13 +0100 Subject: [PATCH] sokol: Proper resource management using refcount Fixes crash when moving frame caused by BuildingInstaller freeing SokolTexture2D while a created command held a ptr to it, causing use-after-free --- Source/Render/D3D/D3DRender.cpp | 1 - Source/Render/sokol/SokolRender.cpp | 27 ++++--- Source/Render/sokol/SokolRender.h | 10 +-- Source/Render/sokol/SokolRenderState.cpp | 97 ++++++++++++------------ Source/Render/sokol/SokolResources.cpp | 51 ++++++++++--- Source/Render/sokol/SokolResources.h | 63 ++++++++++++--- Source/Render/sokol/SokolTypes.h | 4 + Source/Render/src/MeshBank.cpp | 1 - Source/Render/src/RenderDevice.cpp | 1 + 9 files changed, 168 insertions(+), 87 deletions(-) diff --git a/Source/Render/D3D/D3DRender.cpp b/Source/Render/D3D/D3DRender.cpp index 192185ca..89ab8886 100644 --- a/Source/Render/D3D/D3DRender.cpp +++ b/Source/Render/D3D/D3DRender.cpp @@ -1437,7 +1437,6 @@ void cD3DRender::SubmitBuffers(ePrimitiveType primitive, VertexBuffer* vb, size_ xassert((range->offset + range->len) <= vertices); vertices = range->len; } - vertices = range ? range->len : vertices; xassert(0); RDCALL(gb_RenderDevice3D->lpD3DDevice->DrawPrimitive(d3dType, offset, vertices)); } diff --git a/Source/Render/sokol/SokolRender.cpp b/Source/Render/sokol/SokolRender.cpp index c064c9c6..5217a049 100644 --- a/Source/Render/sokol/SokolRender.cpp +++ b/Source/Render/sokol/SokolRender.cpp @@ -194,6 +194,7 @@ int cSokolRender::Done() { RenderSubmitEvent(RenderEvent::DONE, "Sokol start"); bool do_sg_shutdown = sdl_window != nullptr; int ret = cInterfaceRenderDevice::Done(); + activeCommand.Clear(); ClearCommands(); ClearPipelines(); shaders.clear(); @@ -344,16 +345,14 @@ void SokolCommand::CreateShaderParams() { } void SokolCommand::ClearDrawData() { - if (owned_vertex_buffer) { - delete vertex_buffer; - owned_vertex_buffer = false; + if (vertex_buffer) { + vertex_buffer->DecRef(); + vertex_buffer = nullptr; } - if (owned_index_buffer) { - delete index_buffer; - owned_index_buffer = false; + if (index_buffer) { + index_buffer->DecRef(); + index_buffer = nullptr; } - vertex_buffer = nullptr; - index_buffer = nullptr; vertices = 0; indices = 0; @@ -386,7 +385,7 @@ void SokolCommand::ClearShaderParams() { void SokolCommand::ClearTextures() { for (int i = 0; i < PERIMETER_SOKOL_TEXTURES; ++i) { - sokol_textures[i] = nullptr; + SetTexture(i, nullptr); } } @@ -396,7 +395,13 @@ void SokolCommand::Clear() { ClearTextures(); } -void SokolCommand::SetTexture(size_t index, SokolTexture2D* sokol_texture) { +void SokolCommand::SetTexture(size_t index, SokolResourceTexture* texture) { xassert(indexIncRef(); + } + if (sokol_textures[index]) { + sokol_textures[index]->DecRef(); + } + sokol_textures[index] = texture; } diff --git a/Source/Render/sokol/SokolRender.h b/Source/Render/sokol/SokolRender.h index 26462e0e..8c5213e1 100644 --- a/Source/Render/sokol/SokolRender.h +++ b/Source/Render/sokol/SokolRender.h @@ -25,7 +25,7 @@ struct SokolCommand { void Clear(); void ClearDrawData(); void ClearShaderParams(); - void SetTexture(size_t index, SokolTexture2D* sokol_texture); + void SetTexture(size_t index, SokolResource* sokol_texture); void ClearTextures(); NO_COPY_CONSTRUCTOR(SokolCommand) @@ -34,11 +34,9 @@ struct SokolCommand { size_t base_elements = 0; size_t vertices = 0; size_t indices = 0; - struct SokolTexture2D* sokol_textures[PERIMETER_SOKOL_TEXTURES] = {}; - bool owned_vertex_buffer = false; - bool owned_index_buffer = false; - struct SokolBuffer* vertex_buffer = nullptr; - struct SokolBuffer* index_buffer = nullptr; + SokolResourceTexture* sokol_textures[PERIMETER_SOKOL_TEXTURES] = {}; + SokolResourceBuffer* vertex_buffer = nullptr; + SokolResourceBuffer* index_buffer = nullptr; void* vs_params = nullptr; void* fs_params = nullptr; size_t vs_params_len = 0; diff --git a/Source/Render/sokol/SokolRenderState.cpp b/Source/Render/sokol/SokolRenderState.cpp index 8db87f31..30edb71f 100644 --- a/Source/Render/sokol/SokolRenderState.cpp +++ b/Source/Render/sokol/SokolRenderState.cpp @@ -130,48 +130,38 @@ int cSokolRender::EndScene() { continue; } #ifdef PERIMETER_DEBUG - if (sg_query_buffer_state(command->vertex_buffer->buffer) != SG_RESOURCESTATE_VALID) { + if (sg_query_buffer_state(command->vertex_buffer->res) != SG_RESOURCESTATE_VALID) { xxassert(0, "cSokolRender::EndScene vertex_buffer not valid state"); continue; } #endif - bindings.vertex_buffers[0] = command->vertex_buffer->buffer; + bindings.vertex_buffers[0] = command->vertex_buffer->res; xassert(command->indices); if (!command->index_buffer) { xxassert(0, "cSokolRender::EndScene missing index_buffer"); continue; } #ifdef PERIMETER_DEBUG - if (sg_query_buffer_state(command->index_buffer->buffer) != SG_RESOURCESTATE_VALID) { + if (sg_query_buffer_state(command->index_buffer->res) != SG_RESOURCESTATE_VALID) { xxassert(0, "cSokolRender::EndScene index_buffer not valid state"); continue; } #endif - bindings.index_buffer = command->index_buffer->buffer; + bindings.index_buffer = command->index_buffer->res; bindings.fs.samplers[pipeline->shader_fs_sampler_slot] = sampler; //Bind images for (int i = 0; i < PERIMETER_SOKOL_TEXTURES; ++i) { int fs_slot = pipeline->shader_fs_texture_slot[i]; if (fs_slot < 0) continue; - SokolTexture2D* tex = command->sokol_textures[i]; - if (!tex) { - tex = emptyTexture; - if (!tex) { - xxassert(0, "cSokolRender::EndScene sampler tex missing"); - continue; - } - } - if (tex->dirty) { - tex->update(); - } + SokolResourceTexture* tex = command->sokol_textures[i]; #ifdef PERIMETER_DEBUG - if (sg_query_image_state(tex->image) != SG_RESOURCESTATE_VALID) { + if (sg_query_image_state(tex->res) != SG_RESOURCESTATE_VALID) { xxassert(0, "cSokolRender::EndScene sampler tex not valid state"); continue; } #endif - bindings.fs.images[fs_slot] = tex->image; + bindings.fs.images[fs_slot] = tex->res; } sg_apply_bindings(&bindings); @@ -272,7 +262,7 @@ int cSokolRender::Flush(bool wnd) { return 0; } -SokolBuffer* CreateSokolBuffer(MemoryResource* resource, size_t len, bool dynamic, sg_buffer_type type) { +void CreateSokolBuffer(SokolBuffer*& buffer_ptr, MemoryResource* resource, size_t len, bool dynamic, sg_buffer_type type) { MT_IS_GRAPH(); xassert(!resource->locked); xassert(len <= resource->data_len); @@ -288,6 +278,7 @@ SokolBuffer* CreateSokolBuffer(MemoryResource* resource, size_t len, bool dynami desc.label = "CreateSokolBuffer"; } if (desc.usage == SG_USAGE_IMMUTABLE) { + xassert(buffer_ptr == nullptr); xassert(resource->data); xassert(!resource->burned); desc.data = {resource->data, len}; @@ -297,9 +288,16 @@ SokolBuffer* CreateSokolBuffer(MemoryResource* resource, size_t len, bool dynami resource->dirty = true; } - SokolBuffer* buffer = new SokolBuffer(&desc); - - return buffer; + if (buffer_ptr == nullptr) { + sg_buffer buffer = sg_make_buffer(&desc); + buffer_ptr = new SokolBuffer(buffer); + } else if (buffer_ptr->buffer == nullptr) { + //Buffer exists but the resource no, recreate it + sg_buffer buffer = sg_make_buffer(&desc); + buffer_ptr->buffer = new SokolResourceBuffer(buffer); + } else { + xassert(0); + } } void cSokolRender::FinishActiveDrawBuffer() { @@ -357,10 +355,7 @@ void cSokolRender::CreateCommand(VertexBuffer* vb, size_t vertices, IndexBuffer* #ifdef PERIMETER_DEBUG if (vb->fmt & VERTEX_FMT_TEX1) { - if (activeCommand.sokol_textures[0] != emptyTexture) { - xassert(activeCommand.sokol_textures[0]); - } - + xassert(activeCommand.sokol_textures[0]); } if (vb->fmt & VERTEX_FMT_TEX2) { xassert(activeCommand.sokol_textures[1]); @@ -370,8 +365,8 @@ void cSokolRender::CreateCommand(VertexBuffer* vb, size_t vertices, IndexBuffer* //Update buffers if (vb->data && (vb->sg == nullptr || vb->dirty)) { size_t len = vertices * vb->VertexSize; - if (vb->sg == nullptr) { - vb->sg = CreateSokolBuffer(vb, len, vb->dynamic, SG_BUFFERTYPE_VERTEXBUFFER); + if (vb->sg == nullptr || vb->sg->buffer == nullptr) { + CreateSokolBuffer(vb->sg, vb, len, vb->dynamic, SG_BUFFERTYPE_VERTEXBUFFER); } if (vb->dynamic) { vb->sg->update(vb, len); @@ -379,8 +374,8 @@ void cSokolRender::CreateCommand(VertexBuffer* vb, size_t vertices, IndexBuffer* } if (ib->data && (!ib->sg || ib->dirty)) { size_t len = indices * sizeof(indices_t); - if (ib->sg == nullptr) { - ib->sg = CreateSokolBuffer(ib, len, ib->dynamic, SG_BUFFERTYPE_INDEXBUFFER); + if (ib->sg == nullptr || ib->sg->buffer == nullptr) { + CreateSokolBuffer(ib->sg, ib, len, ib->dynamic, SG_BUFFERTYPE_INDEXBUFFER); } if (ib->dynamic) { ib->sg->update(ib, len); @@ -448,18 +443,10 @@ void cSokolRender::CreateCommand(VertexBuffer* vb, size_t vertices, IndexBuffer* } //Transfer buffers to command - cmd->owned_vertex_buffer = vb->dynamic; - cmd->owned_index_buffer = ib->dynamic; - cmd->vertex_buffer = vb->sg; - cmd->index_buffer = ib->sg; - if (cmd->owned_vertex_buffer) { - vb->sg = nullptr; - vb->burned = false; - } - if (cmd->owned_index_buffer) { - ib->sg = nullptr; - ib->burned = false; - } + vb->sg->buffer->IncRef(); + ib->sg->buffer->IncRef(); + cmd->vertex_buffer = vb->sg->buffer; + cmd->index_buffer = ib->sg->buffer; activeCommand.base_elements = 0; activeCommand.vertices = 0; activeCommand.indices = 0; @@ -498,6 +485,19 @@ void cSokolRender::SetActiveDrawBuffer(DrawBuffer* db) { activeCommand.base_elements = 0; activeCommand.vertices = 0; activeCommand.indices = 0; + //Those that are dynamic should have their buffer released and unburned since we wil recreate later + if (db->vb.dynamic && db->vb.burned) { + db->vb.burned = false; + if (db->vb.sg) { + db->vb.sg->release_buffer(); + } + } + if (db->ib.dynamic && db->ib.burned) { + db->ib.burned = false; + if (db->ib.sg) { + db->ib.sg->release_buffer(); + } + } } void cSokolRender::SubmitDrawBuffer(DrawBuffer* db, DrawRange* range) { @@ -638,14 +638,17 @@ void cSokolRender::SetBlendState(eBlendMode blend) { void cSokolRender::SetTextureImage(uint32_t slot, TextureImage* texture_image) { xassert(slot < GetMaxTextureSlots()); - SokolTexture2D* tex = texture_image ? texture_image->sg : nullptr; - //Required as sometimes empty slot must be used with color_tex1 shader - if (!tex && slot == 0) { - tex = emptyTexture; + SokolTexture2D* tex = texture_image == nullptr ? emptyTexture : texture_image->sg; + if (!tex) { + xxassert(0, "cSokolRender::EndScene sampler tex missing"); + return; + } + if (tex->dirty) { + tex->update(); } - if (activeCommand.sokol_textures[slot] != tex) { + if (activeCommand.sokol_textures[slot] != tex->image) { FinishActiveDrawBuffer(); - activeCommand.SetTexture(slot, tex); + activeCommand.SetTexture(slot, tex->image); } } diff --git a/Source/Render/sokol/SokolResources.cpp b/Source/Render/sokol/SokolResources.cpp index 1ad27730..1a088d77 100644 --- a/Source/Render/sokol/SokolResources.cpp +++ b/Source/Render/sokol/SokolResources.cpp @@ -2,6 +2,7 @@ #include "xmath.h" #include #include "SokolResources.h" +#include "SokolTypes.h" size_t sokol_pixelformat_bytesize(sg_pixel_format fmt) { //Probably the only pixel format used, so we cache it @@ -12,18 +13,38 @@ size_t sokol_pixelformat_bytesize(sg_pixel_format fmt) { return sg_query_pixelformat(fmt).bytes_per_pixel; } -SokolBuffer::SokolBuffer(sg_buffer_desc* desc) -: buffer(sg_make_buffer(desc)) { - xassert(buffer.id != SG_INVALID_ID); +template<> +void SokolResourceBuffer::destroy_res() { + if (res.id != SG_INVALID_ID) { + sg_destroy_buffer(res); + res.id = SG_INVALID_ID; + } +} + +template<> +void SokolResourceTexture::destroy_res() { + if (res.id != SG_INVALID_ID) { + sg_destroy_image(res); + res.id = SG_INVALID_ID; + } +} + +SokolBuffer::SokolBuffer(sg_buffer _buffer) { + buffer = new SokolResource(_buffer); } SokolBuffer::~SokolBuffer() { - if (buffer.id != SG_INVALID_ID) { - sg_destroy_buffer(buffer); + release_buffer(); +} + +void SokolBuffer::release_buffer() { + if (buffer) { + buffer->DecRef(); + buffer = nullptr; } } -void SokolBuffer::update(MemoryResource* resource, size_t len) { +void SokolBuffer::update(MemoryResource* resource, size_t len) const { xassert(!resource->burned); xassert(!resource->locked); if (!resource->dirty) return; @@ -36,8 +57,13 @@ void SokolBuffer::update(MemoryResource* resource, size_t len) { resource->burned = true; resource->dirty = false; + if (!buffer) { + xassert(0); + return; + } + xassert(buffer->res.id != SG_INVALID_ID); sg_range range = {resource->data, len}; - sg_update_buffer(buffer, &range); + sg_update_buffer(buffer->res, &range); } SokolTexture2D::SokolTexture2D(sg_image_desc* _desc) @@ -48,7 +74,10 @@ SokolTexture2D::SokolTexture2D(sg_image_desc* _desc) } SokolTexture2D::~SokolTexture2D() { - if (image.id != SG_INVALID_ID) sg_destroy_image(image); + if (image) { + image->DecRef(); + image = nullptr; + } delete desc; } @@ -64,8 +93,7 @@ void SokolTexture2D::update() { } #endif xassert(desc->usage == SG_USAGE_IMMUTABLE || data); - image = sg_make_image(desc); - xassert(image.id != SG_INVALID_ID); + image = new SokolResource(sg_make_image(desc)); if (desc->usage == SG_USAGE_IMMUTABLE) { //Cleanup subimages for (int ci = 0; ci < SG_CUBEFACE_NUM; ++ci) { @@ -85,8 +113,9 @@ void SokolTexture2D::update() { } if (data) { + xassert(image->res.id != SG_INVALID_ID); sg_image_data imageData; imageData.subimage[0][0] = {data, data_len}; - sg_update_image(image, &imageData); + sg_update_image(image->res, &imageData); } } diff --git a/Source/Render/sokol/SokolResources.h b/Source/Render/sokol/SokolResources.h index f57af9a4..483f3c74 100644 --- a/Source/Render/sokol/SokolResources.h +++ b/Source/Render/sokol/SokolResources.h @@ -1,24 +1,67 @@ #ifndef PERIMETER_SOKOLRESOURCES_H #define PERIMETER_SOKOLRESOURCES_H - + /* - * Stores sokol resources reference, malloc allocated memory and dirty flag + * Stuff related to sokol resource management * - * sokol resources can only be updated at most once per per frame, this way we can update several times + * Sokol resources can only be updated at most once per per frame, + * by storing the data on memory we can update several times until final snapshot is sent + * to sokol resource and then into SokolCommand */ #include "MemoryResource.h" size_t sokol_pixelformat_bytesize(sg_pixel_format fmt); -struct SokolBuffer { - sg_buffer buffer = {}; +/** + * Stores sokol resource reference with a refcount + */ +template +class SokolResource { +private: + int32_t refcount = 1; + + //Has to be implemented by template specialization for each T type + void destroy_res(); - explicit SokolBuffer(sg_buffer_desc* desc); - NO_COPY_CONSTRUCTOR(SokolBuffer) - ~SokolBuffer(); + ~SokolResource() { + if (0 < refcount) { + xassert(refcount == 1); + DecRef(); + } + } +public: + T res; + + explicit SokolResource(T res) : res(res) { + xassert(res.id != SG_INVALID_ID); + } + + NO_COPY_CONSTRUCTOR(SokolResource) - void update(MemoryResource* resource, size_t len); + int32_t IncRef() { + return ++refcount; + } + + int32_t DecRef() { + if (0 < refcount) { + refcount--; + if (refcount == 0) { + destroy_res(); + delete this; + } + } + return refcount; + } +}; + +struct SokolBuffer { + SokolResource* buffer = nullptr; + explicit SokolBuffer(sg_buffer buffer); + ~SokolBuffer(); + + void release_buffer(); + void update(MemoryResource* resource, size_t len) const; }; struct SokolTexture2D : MemoryResource { @@ -27,7 +70,7 @@ struct SokolTexture2D : MemoryResource { #endif sg_pixel_format pixel_format; sg_image_desc* desc = nullptr; - sg_image image = {}; + SokolResource* image = nullptr; explicit SokolTexture2D(sg_image_desc* desc); NO_COPY_CONSTRUCTOR(SokolTexture2D) diff --git a/Source/Render/sokol/SokolTypes.h b/Source/Render/sokol/SokolTypes.h index 02b3b918..21b943bc 100644 --- a/Source/Render/sokol/SokolTypes.h +++ b/Source/Render/sokol/SokolTypes.h @@ -40,4 +40,8 @@ struct PIPELINE_MODE { void FromValue(pipeline_mode_value_t value); }; +template class SokolResource; +using SokolResourceTexture = SokolResource; +using SokolResourceBuffer = SokolResource; + #endif //PERIMETER_SOKOLTYPES_H diff --git a/Source/Render/src/MeshBank.cpp b/Source/Render/src/MeshBank.cpp index d7c1f3d8..939f4a77 100644 --- a/Source/Render/src/MeshBank.cpp +++ b/Source/Render/src/MeshBank.cpp @@ -204,7 +204,6 @@ void cMeshStatic::EndBuildMesh(bool bump) } #endif db.Create(n_vertex, false, n_indices, false, fmt, PT_TRIANGLES); - db.set_as_active = false; void* pVertex = nullptr; indices_t* indices = nullptr; diff --git a/Source/Render/src/RenderDevice.cpp b/Source/Render/src/RenderDevice.cpp index a9377b34..2bfb2adb 100644 --- a/Source/Render/src/RenderDevice.cpp +++ b/Source/Render/src/RenderDevice.cpp @@ -298,6 +298,7 @@ DrawBuffer* cInterfaceRenderDevice::GetDrawBuffer(vertex_fmt_t fmt, ePrimitiveTy drawBuffers.resize(key + 1); } if (!db) { + //No drawbuffer exists for this key, create new one db = new DrawBuffer(); db->Create(PERIMETER_RENDER_VERTEXBUF_LEN, true, PERIMETER_RENDER_INDEXBUF_LEN, true, fmt, primitive); drawBuffers[key] = db;