From 30ccac3a02adce1dcb1b9ae7425ca74d44e2c54d Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Thu, 21 Nov 2024 17:30:27 +0100 Subject: [PATCH] buffer: introduce wlr_buffer_acquire() A footgun in the wlr_buffer API is that there's no difference between acquiring a buffer and increasing the lock count. In other words, transitioning a buffer from the released state to the acquired state is not explicit. This may result in hard-to-debug failures if there is a dangling released wlr_buffer somewhere (e.g. wlr_client_buffer.source [1]) and some piece of code calls wlr_buffer_lock(). In that case, the buffer will be acquired and released again. In the context of a wlr_buffer issued from a Wayland protocol wl_buffer object, this can cause the underlying memory to be used after wl_buffer.release has been sent to the client, and a double wl_buffer.release event to be sent. Make it so acquiring a buffer is an explicit operation to make sure the caller means the state transition and is prepared for a new release event. wlr_buffer_acquire() forbids calls on already-acquired buffers, and wlr_buffer_lock() now forbids calls on released buffers. [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4904 --- include/wlr/types/wlr_buffer.h | 18 +++++++++++++++--- render/swapchain.c | 2 +- types/buffer/buffer.c | 7 +++++++ types/buffer/client.c | 2 +- types/buffer/resource.c | 9 ++++++++- 5 files changed, 32 insertions(+), 6 deletions(-) diff --git a/include/wlr/types/wlr_buffer.h b/include/wlr/types/wlr_buffer.h index fadc560a3..2be32ffd4 100644 --- a/include/wlr/types/wlr_buffer.h +++ b/include/wlr/types/wlr_buffer.h @@ -44,9 +44,11 @@ enum wlr_buffer_cap { * A buffer containing pixel data. * * A buffer has a single producer (the party who created the buffer) and - * multiple consumers (parties reading the buffer). When all consumers are done - * with the buffer, it gets released and can be re-used by the producer. When - * the producer and all consumers are done with the buffer, it gets destroyed. + * multiple consumers (parties reading the buffer). Initially, a buffer is + * released. When the consumer passes the buffer to a consumer, the buffer is + * acquired. When all consumers are done with the buffer, it gets released and + * can be re-used by the producer. When the producer and all consumers are done + * with the buffer, it gets destroyed. */ struct wlr_buffer { const struct wlr_buffer_impl *impl; @@ -70,10 +72,20 @@ struct wlr_buffer { * they are done with the buffer. */ void wlr_buffer_drop(struct wlr_buffer *buffer); +/** + * Acquire the buffer. This function should be called by producers when they + * pass a released buffer to a consumer. The consumer is responsible for + * calling wlr_buffer_unlock() once they are done with the buffer. + * + * This function aborts if the buffer has already been acquired. + */ +struct wlr_buffer *wlr_buffer_acquire(struct wlr_buffer *buffer); /** * Lock the buffer. This function should be called by consumers to make * sure the buffer can be safely read from. Once the consumer is done with the * buffer, they should call wlr_buffer_unlock(). + * + * This function aborts if the buffer hasn't been acquired. */ struct wlr_buffer *wlr_buffer_lock(struct wlr_buffer *buffer); /** diff --git a/render/swapchain.c b/render/swapchain.c index e87a99ead..04d2fcd28 100644 --- a/render/swapchain.c +++ b/render/swapchain.c @@ -74,7 +74,7 @@ static struct wlr_buffer *slot_acquire(struct wlr_swapchain *swapchain, slot->release.notify = slot_handle_release; wl_signal_add(&slot->buffer->events.release, &slot->release); - return wlr_buffer_lock(slot->buffer); + return wlr_buffer_acquire(slot->buffer); } struct wlr_buffer *wlr_swapchain_acquire(struct wlr_swapchain *swapchain) { diff --git a/types/buffer/buffer.c b/types/buffer/buffer.c index 953207a2c..e360100ac 100644 --- a/types/buffer/buffer.c +++ b/types/buffer/buffer.c @@ -45,7 +45,14 @@ void wlr_buffer_drop(struct wlr_buffer *buffer) { buffer_consider_destroy(buffer); } +struct wlr_buffer *wlr_buffer_acquire(struct wlr_buffer *buffer) { + assert(buffer->n_locks == 0); + buffer->n_locks++; + return buffer; +} + struct wlr_buffer *wlr_buffer_lock(struct wlr_buffer *buffer) { + assert(buffer->n_locks > 0); buffer->n_locks++; return buffer; } diff --git a/types/buffer/client.c b/types/buffer/client.c index d96603a45..b9480e2f9 100644 --- a/types/buffer/client.c +++ b/types/buffer/client.c @@ -89,7 +89,7 @@ struct wlr_client_buffer *wlr_client_buffer_create(struct wlr_buffer *buffer, client_buffer->renderer_destroy.notify = client_buffer_handle_renderer_destroy; // Ensure the buffer will be released before being destroyed - wlr_buffer_lock(&client_buffer->base); + wlr_buffer_acquire(&client_buffer->base); wlr_buffer_drop(&client_buffer->base); return client_buffer; diff --git a/types/buffer/resource.c b/types/buffer/resource.c index 14d8f82d1..fc7d29162 100644 --- a/types/buffer/resource.c +++ b/types/buffer/resource.c @@ -57,5 +57,12 @@ struct wlr_buffer *wlr_buffer_try_from_resource(struct wl_resource *resource) { return NULL; } - return wlr_buffer_lock(buffer); + // If the buffer is released, we want to acquire it. If the buffer is + // already acquired (e.g. because it has been attached to another surface), + // we want to lock it. + if (buffer->n_locks > 0) { + return wlr_buffer_lock(buffer); + } else { + return wlr_buffer_acquire(buffer); + } }