From 0deef6fe44a939a47a170fa8eae55c9ea08520d9 Mon Sep 17 00:00:00 2001 From: Isaac Freund Date: Fri, 3 Jun 2022 00:15:42 +0200 Subject: [PATCH] output: fix leak of empty back buffer lock This refactors output_ensure_buffer() to not mutate the state passed, making the previous subtle behavior much more explicit. Fixes: d483dd2f ("output: add wlr_output_commit_state") Closes: #3442 --- include/types/wlr_output.h | 2 +- types/output/output.c | 51 +++++++++++++++++++++++---------- types/output/render.c | 58 ++++++++++++++++++++++++++------------ 3 files changed, 77 insertions(+), 34 deletions(-) diff --git a/include/types/wlr_output.h b/include/types/wlr_output.h index 9223407a2..5f0cbe337 100644 --- a/include/types/wlr_output.h +++ b/include/types/wlr_output.h @@ -13,6 +13,6 @@ struct wlr_drm_format *output_pick_format(struct wlr_output *output, const struct wlr_drm_format_set *display_formats, uint32_t format); void output_clear_back_buffer(struct wlr_output *output); bool output_ensure_buffer(struct wlr_output *output, - struct wlr_output_state *state); + const struct wlr_output_state *state, bool *new_back_buffer); #endif diff --git a/types/output/output.c b/types/output/output.c index c69b517ff..a4cb6f740 100644 --- a/types/output/output.c +++ b/types/output/output.c @@ -677,22 +677,29 @@ static bool output_basic_test(struct wlr_output *output, bool wlr_output_test_state(struct wlr_output *output, const struct wlr_output_state *state) { - bool had_buffer = state->committed & WLR_OUTPUT_STATE_BUFFER; - - // Duplicate the state because we might mutate it in output_ensure_buffer - struct wlr_output_state pending = *state; - if (!output_basic_test(output, &pending)) { - return false; - } - if (!output_ensure_buffer(output, &pending)) { + if (!output_basic_test(output, state)) { return false; } if (!output->impl->test) { return true; } - bool success = output->impl->test(output, &pending); - if (!had_buffer) { + bool new_back_buffer = false; + if (!output_ensure_buffer(output, state, &new_back_buffer)) { + return false; + } + + // Create a shallow copy of the state with the new buffer + // potentially included to pass to the backend. + struct wlr_output_state copy = *state; + if (new_back_buffer) { + assert((copy.committed & WLR_OUTPUT_STATE_BUFFER) == 0); + copy.committed |= WLR_OUTPUT_STATE_BUFFER; + copy.buffer = output->back_buffer; + } + + bool success = output->impl->test(output, ©); + if (new_back_buffer) { output_clear_back_buffer(output); } return success; @@ -710,13 +717,23 @@ bool wlr_output_commit_state(struct wlr_output *output, return false; } - // Duplicate the state because we might mutate it in output_ensure_buffer - struct wlr_output_state pending = *state; - if (!output_ensure_buffer(output, &pending)) { + bool new_back_buffer = false; + if (!output_ensure_buffer(output, state, &new_back_buffer)) { output_clear_back_buffer(output); return false; } + // Create a shallow copy of the state with the new back buffer + // potentially included to pass to the backend. + struct wlr_output_state pending = *state; + if (new_back_buffer) { + assert((pending.committed & WLR_OUTPUT_STATE_BUFFER) == 0); + pending.committed |= WLR_OUTPUT_STATE_BUFFER; + // Lock the buffer to ensure it stays valid past the + // output_clear_back_buffer() call below. + pending.buffer = wlr_buffer_lock(output->back_buffer); + } + if ((pending.committed & WLR_OUTPUT_STATE_BUFFER) && output->idle_frame != NULL) { wl_event_source_remove(output->idle_frame); @@ -746,6 +763,9 @@ bool wlr_output_commit_state(struct wlr_output *output, if (!output->impl->commit(output, &pending)) { wlr_buffer_unlock(back_buffer); + if (new_back_buffer) { + wlr_buffer_unlock(pending.buffer); + } return false; } @@ -820,8 +840,9 @@ bool wlr_output_commit_state(struct wlr_output *output, }; wlr_signal_emit_safe(&output->events.commit, &event); - if (back_buffer != NULL) { - wlr_buffer_unlock(back_buffer); + wlr_buffer_unlock(back_buffer); + if (new_back_buffer) { + wlr_buffer_unlock(pending.buffer); } return true; diff --git a/types/output/render.c b/types/output/render.c index ab586d645..2e38919a1 100644 --- a/types/output/render.c +++ b/types/output/render.c @@ -99,7 +99,7 @@ static bool output_create_swapchain(struct wlr_output *output, } static bool output_attach_back_buffer(struct wlr_output *output, - struct wlr_output_state *state, int *buffer_age) { + const struct wlr_output_state *state, int *buffer_age) { assert(output->back_buffer == NULL); if (!output_create_swapchain(output, state, true)) { @@ -151,11 +151,11 @@ bool wlr_output_attach_render(struct wlr_output *output, int *buffer_age) { return output_attach_render(output, &output->pending, buffer_age); } -static bool output_attach_empty_buffer(struct wlr_output *output, - struct wlr_output_state *state) { +static bool output_attach_empty_back_buffer(struct wlr_output *output, + const struct wlr_output_state *state) { assert(!(state->committed & WLR_OUTPUT_STATE_BUFFER)); - if (!output_attach_render(output, state, NULL)) { + if (!output_attach_back_buffer(output, state, NULL)) { return false; } @@ -170,8 +170,28 @@ static bool output_attach_empty_buffer(struct wlr_output *output, return true; } +static bool output_test_with_back_buffer(struct wlr_output *output, + const struct wlr_output_state *state) { + assert(output->impl->test != NULL); + + // Create a shallow copy of the state with the empty back buffer included + // to pass to the backend. + struct wlr_output_state copy = *state; + assert((copy.committed & WLR_OUTPUT_STATE_BUFFER) == 0); + copy.committed |= WLR_OUTPUT_STATE_BUFFER; + assert(output->back_buffer != NULL); + copy.buffer = output->back_buffer; + + return output->impl->test(output, ©); +} + +// This function may attach a new, empty back buffer if necessary. +// If so, the new_back_buffer out parameter will be set to true. bool output_ensure_buffer(struct wlr_output *output, - struct wlr_output_state *state) { + const struct wlr_output_state *state, + bool *new_back_buffer) { + assert(*new_back_buffer == false); + // If we're lighting up an output or changing its mode, make sure to // provide a new buffer bool needs_new_buffer = false; @@ -196,15 +216,16 @@ bool output_ensure_buffer(struct wlr_output *output, wlr_log(WLR_DEBUG, "Attaching empty buffer to output for modeset"); - if (!output_attach_empty_buffer(output, state)) { - goto error; + if (!output_attach_empty_back_buffer(output, state)) { + return false; } - if (!output->impl->test || output->impl->test(output, state)) { + + if (output_test_with_back_buffer(output, state)) { + *new_back_buffer = true; return true; } output_clear_back_buffer(output); - state->committed &= ~WLR_OUTPUT_STATE_BUFFER; if (output->swapchain->format->len == 0) { return false; @@ -217,17 +238,18 @@ bool output_ensure_buffer(struct wlr_output *output, if (!output_create_swapchain(output, state, false)) { return false; } - if (!output_attach_empty_buffer(output, state)) { - goto error; - } - if (!output->impl->test(output, state)) { - goto error; - } - return true; -error: + if (!output_attach_empty_back_buffer(output, state)) { + return false; + } + + if (output_test_with_back_buffer(output, state)) { + *new_back_buffer = true; + return true; + } + output_clear_back_buffer(output); - state->committed &= ~WLR_OUTPUT_STATE_BUFFER; + return false; }