From fc6533c92048029032eaa734ff538b4fccb94c03 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= <daniel@ekloef.se>
Date: Wed, 15 Dec 2021 12:37:21 +0100
Subject: [PATCH] shm: get_buffer(): make sure buffer->busy is set
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When going through the cached buffers, we only set buffer->busy on
the *first* re-usable buffer we found.

In some cases, we will find more than one re-usable buffer. In this
case, we select the “youngest” one (i.e the one most recently used, in
the hopes that we can use damage tracking instead of re-rendering the
entire buffer).

If the “current” buffer is younger than the previously detected,
re-usable, buffer, then we unref:ed the previously selected buffer,
and replaced it with the current one.

But, we did not sanitize it. That is, we did not:

* set buffer->busy
* clear its dirty region
* clear its scroll damage

That buffer would eventually get rendered to, and committed to the
compositor. Later, the compositor would free it. And there, in our
buffer_release() callback, we’d assert that buffer->busy was
set. And fail.

Closes #844
---
 CHANGELOG.md |  2 ++
 shm.c        | 29 +++++++++++++++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 74313984..1904fd5d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -60,6 +60,8 @@
   (https://codeberg.org/dnkl/foot/issues/842).
 * Key presses triggering keyboard layout switches also being emitted
   CSI codes in the Kitty keyboard protocol.
+* Assertion in `shm.c:buffer_release()`
+  (https://codeberg.org/dnkl/foot/issues/844).
 
 
 ### Security
diff --git a/shm.c b/shm.c
index 9db7e745..62afd76d 100644
--- a/shm.c
+++ b/shm.c
@@ -546,15 +546,9 @@ shm_get_buffer(struct buffer_chain *chain, int width, int height)
             else
 #endif
             {
-                if (cached == NULL) {
-                    LOG_DBG("re-using buffer %p from cache", (void *)buf);
-                    buf->busy = true;
-                    pixman_region32_clear(&buf->public.dirty);
-                    free(buf->public.scroll_damage);
-                    buf->public.scroll_damage = NULL;
-                    xassert(buf->public.pix_instances == chain->pix_instances);
-                    cached = it->item;
-                } else {
+                if (cached == NULL)
+                    cached = buf;
+                else {
                     /* We have multiple buffers eligible for
                      * re-use. Pick the “youngest” one, and mark the
                      * other one for purging */
@@ -562,6 +556,14 @@ shm_get_buffer(struct buffer_chain *chain, int width, int height)
                         shm_unref(&cached->public);
                         cached = buf;
                     } else {
+                        /*
+                         * TODO: I think we _can_ use shm_unref()
+                         * here...
+                         *
+                         * shm_unref() may remove ‘it’, but that
+                         * should be safe; “our” tll_foreach() already
+                         * holds the next pointer.
+                         */
                         if (buffer_unref_no_remove_from_chain(buf))
                             tll_remove(chain->bufs, it);
                     }
@@ -569,8 +571,15 @@ shm_get_buffer(struct buffer_chain *chain, int width, int height)
             }
     }
 
-    if (cached != NULL)
+    if (cached != NULL) {
+        LOG_DBG("re-using buffer %p from cache", (void *)cached);
+        cached->busy = true;
+        pixman_region32_clear(&cached->public.dirty);
+        free(cached->public.scroll_damage);
+        cached->public.scroll_damage = NULL;
+        xassert(cached->public.pix_instances == chain->pix_instances);
         return &cached->public;
+    }
 
     struct buffer *ret;
     get_new_buffers(chain, 1, &width, &height, &ret, false);