Skip to content

Commit

Permalink
Fix possible crash with Font:getLines and Pass:text;
Browse files Browse the repository at this point in the history
- Font:getLines/Pass:text use temp memory for strings/vertices.
- Due to the recent morgue fix, resizing the atlas will now do a GPU
  submit to flush the transfers before destroying the atlas.
- This GPU submit also rewinds the temp allocator, invalidating the
  temp memory used for the lines/vertices, causing a use-after-free.

There are 2 changes here:

- Only flush transfers if the destroyed resource actually has pending
  work (we're already tracking this w/ lastTransferRead/Write).
- Restore the allocator cursor after doing the submit.

Having to restore the allocator offset is kinda tricky/complicated,
which sucks.  Other solutions might be:

- Avoid using temp memory in those Font methods.  More generally, adopt
  a rule where you can't use temp memory if it's possible that there
  will be a submit while you're using the temp memory.
- Find some way to stop destroying the old font atlas during a resize?
- Don't rewind the temp allocator on every GPU submit.  Instead only
  rewind it at the end of a frame, or only when Lua does the submit.
  • Loading branch information
bjornbytes committed Dec 3, 2023
1 parent 987c029 commit fbae4d1
Showing 1 changed file with 13 additions and 5 deletions.
18 changes: 13 additions & 5 deletions src/modules/graphics/graphics.c
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ static gpu_pipeline* getPipeline(uint32_t index);
static MappedBuffer mapBuffer(BufferPool* pool, uint32_t size, size_t align);
static int u64cmp(const void* a, const void* b);
static void beginFrame(void);
static void flushTransfers(void);
static void flushTransfers(Sync* sync);
static void processReadbacks(void);
static size_t getLayout(gpu_slot* slots, uint32_t count);
static gpu_bundle* getBundle(size_t layout, gpu_binding* bindings, uint32_t count);
Expand Down Expand Up @@ -1883,7 +1883,7 @@ Buffer* lovrBufferCreate(const BufferInfo* info, void** data) {

void lovrBufferDestroy(void* ref) {
Buffer* buffer = ref;
flushTransfers();
flushTransfers(&buffer->sync);
gpu_buffer_destroy(buffer->gpu);
free(buffer);
}
Expand Down Expand Up @@ -2267,7 +2267,7 @@ Texture* lovrTextureCreateView(const TextureViewInfo* view) {
void lovrTextureDestroy(void* ref) {
Texture* texture = ref;
if (texture != state.window) {
flushTransfers();
flushTransfers(&texture->sync);
lovrRelease(texture->material, lovrMaterialDestroy);
lovrRelease(texture->info.parent, lovrTextureDestroy);
if (texture->renderView && texture->renderView != texture->gpu) gpu_texture_destroy(texture->renderView);
Expand Down Expand Up @@ -7212,9 +7212,17 @@ static void beginFrame(void) {
processReadbacks();
}

static void flushTransfers(void) {
if (state.active) {
// When a Buffer/Texture is garbage collected, if it has any transfer operations recorded to
// state.stream, those transfers need to be submitted before it gets destroyed. The allocator
// offset is saved and restored, which is pretty gross, but we don't want to invalidate temp memory
// (currently this is only a problem for Font: when the font's atlas gets destroyed, it could
// invalidate the temp memory used by Font:getLines and Pass:text).
static void flushTransfers(Sync* sync) {
if (state.active && (sync->lastTransferRead == state.tick && sync->lastTransferWrite == state.tick)) {
size_t cursor = state.allocator.cursor;
lovrGraphicsSubmit(NULL, 0);
beginFrame();
state.allocator.cursor = cursor;
}
}

Expand Down

0 comments on commit fbae4d1

Please sign in to comment.