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

@@ -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

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;

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->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;
 }