aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorIsaac Freund <mail@isaacfreund.com>2022-06-03 00:15:42 +0200
committerSimon Ser <contact@emersion.fr>2022-06-07 15:30:08 +0000
commit0deef6fe44a939a47a170fa8eae55c9ea08520d9 (patch)
tree6f197dfc964921cafa358e817d77aa4239b9e626
parent5cca72958a37c3474df1975c271d8884cf3ec6d2 (diff)
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
-rw-r--r--include/types/wlr_output.h2
-rw-r--r--types/output/output.c51
-rw-r--r--types/output/render.c54
3 files changed, 75 insertions, 32 deletions
diff --git a/include/types/wlr_output.h b/include/types/wlr_output.h
index 9223407a..5f0cbe33 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 c69b517f..a4cb6f74 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, &copy);
+ 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 ab586d64..2e38919a 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, &copy);
+}
+
+// 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_attach_empty_back_buffer(output, state)) {
+ return false;
}
- if (!output->impl->test(output, state)) {
- goto error;
+
+ if (output_test_with_back_buffer(output, state)) {
+ *new_back_buffer = true;
+ return true;
}
- return true;
-error:
output_clear_back_buffer(output);
- state->committed &= ~WLR_OUTPUT_STATE_BUFFER;
+
return false;
}