From fbae4d19610919e877a8ed9b3955d6ce9b83fd64 Mon Sep 17 00:00:00 2001
From: bjorn <mail@bjornbyt.es>
Date: Sat, 2 Dec 2023 17:11:22 -0800
Subject: [PATCH] Fix possible crash with Font:getLines and Pass:text;

- 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.
---
 src/modules/graphics/graphics.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/modules/graphics/graphics.c b/src/modules/graphics/graphics.c
index aed92e668..816fe6086 100644
--- a/src/modules/graphics/graphics.c
+++ b/src/modules/graphics/graphics.c
@@ -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);
@@ -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);
 }
@@ -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);
@@ -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;
   }
 }