Skip to content

Commit

Permalink
sokol: Proper resource management using refcount
Browse files Browse the repository at this point in the history 
Fixes crash when moving frame caused by BuildingInstaller
freeing SokolTexture2D while a created command held a ptr
to it, causing use-after-free
  • Loading branch information
@IonAgorria
IonAgorria committed Feb 21, 2024
1 parent 6b11b99 commit 4f02905
Show file tree
Hide file tree
Showing 9 changed files with 168 additions and 87 deletions.
1 change: 0 additions & 1 deletion Source/Render/D3D/D3DRender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
27 changes: 16 additions & 11 deletions Source/Render/sokol/SokolRender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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(index<PERIMETER_SOKOL_TEXTURES);
sokol_textures[index] = sokol_texture;
if (texture) {
texture->IncRef();
}
if (sokol_textures[index]) {
sokol_textures[index]->DecRef();
}
sokol_textures[index] = texture;
}
10 changes: 4 additions & 6 deletions Source/Render/sokol/SokolRender.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<sg_image>* sokol_texture);
void ClearTextures();
NO_COPY_CONSTRUCTOR(SokolCommand)

Expand All @@ -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;
Expand Down
97 changes: 50 additions & 47 deletions Source/Render/sokol/SokolRenderState.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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};
Expand All @@ -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() {
Expand Down Expand Up @@ -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]);
Expand All @@ -370,17 +365,17 @@ 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);
}
}
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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
}

Expand Down
51 changes: 40 additions & 11 deletions Source/Render/sokol/SokolResources.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "xmath.h"
#include <sokol_gfx.h>
#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
Expand All @@ -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;
Expand All @@ -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)
Expand All @@ -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;
}

Expand All @@ -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) {
Expand All @@ -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);
}
}
Loading

0 comments on commit 4f02905

Please sign in to comment.