Merge pull request 'gamescope aborted on every connect — a PipeWire use-after-free wearing an "unreachable" assert' (#152) from worktree-gamescope-pipewire-buffer-abort into main

Reviewed-on: #152

packaging/gamescope/PKGBUILD

@@ -19,7 +19,7 @@ pkgname=punktfunk-gamescope
 # bump it with the marker so pacman sees a new version when only our patches moved.
 _gsver=3.16.25
 _gsrev=5fb8dce4a09d0a68d097b9faf9513782106bc843
-pkgver="${_gsver}.pfhdr4"
+pkgver="${_gsver}.pfhdr5"
 # 2: patch 0006 (never destroy the Vulkan device/output at exit). No capability moved, so the
 # `.pfhdrN` level deliberately stays put — see README.md.
 # 3: pin moved 8c676c39 -> 5fb8dce4 (3.16.25-1 -> 3.16.25-11), which brings upstream's own
@@ -28,7 +28,12 @@ pkgver="${_gsver}.pfhdr4"
 # screenshots, which are upstream's paths, not ours. Patch 0001 additionally now offers
 # `xBGR_210LE` BEFORE `xRGB_210LE` so a third-party consumer cannot pick the one NVIDIA fills
 # byte-reversed. Still no capability the host probes for, so `.pfhdrN` stays at 4.
-pkgrel=3
+#
+# pfhdr5 / rel 1: patch 0007 fixes the PipeWire `pw_buffer->user_data` use-after-free that aborted
+# a session on every capture renegotiation — i.e. on every client connect, since the host sets the
+# session to the client's mode. This one DOES move `.pfhdrN`, even though it adds no capability:
+# every deployed pfhdr4 binary crash-loops, so an operator has to be able to tell them apart.
+pkgrel=1
 pkgdesc="gamescope with 10-bit BT.2020/PQ PipeWire capture, for punktfunk HDR streaming"
 arch=('x86_64' 'aarch64')
 url="https://git.unom.io/unom/punktfunk"

packaging/gamescope/README.md

@@ -17,6 +17,7 @@ The patches here add the missing half, and nothing else. See
 | `0004-pipewire-optionally-composite-the-external-overlay-i.patch` | `--pipewire-composite-external-overlay` (off by default): paint the external overlay layer (mangoapp — the fps/stats readout) into the capture stream | **Yes** — same shape as the cursor patch, same argument |
 | `0005-punktfunk-stamp-the-version-banner-with-pfhdrN.patch` | Append `+pfhdr<N>` to the `--version` banner | **No** — ours only, retired when the functional patches above land upstream |
 | `0006-punktfunk-never-destroy-the-Vulkan-device-or-output-.patch` | Give `g_device` and `g_output` storage that is never destroyed, so their destructors cannot call a Vulkan driver glibc has already unloaded at `exit()` | **Yes** — a plain static-destruction-order bug, not punktfunk-specific |
+| `0007-pipewire-never-leave-pw_buffer-user_data-pointing-at.patch` | Associate `pw_buffer->user_data` with its `pipewire_buffer` for every path out of `add_buffer`, clear it in `remove_buffer` (the last point both halves are known), and null-check the consumers — killing the use-after-free that aborted the session on every capture renegotiation | **Yes** — a plain use-after-free in the PipeWire buffer lifecycle |
 
 ### Why the headless patch matters
 
@@ -40,6 +41,32 @@ fixed-function front end with no blend stage. Painting the cursor into the node
 for the blend, and with it a full-frame pass per frame — a gamescope session becomes the first one
 that can be genuinely zero-copy end to end.
 
+### Why the buffer-lifetime patch is the one that made sessions unusable
+
+Patch 0007 is not a refinement — without it a managed gamescope session dies on essentially every
+client connect. The host sets the session to the client's mode, the mode change renegotiates the
+PipeWire stream, and the renegotiation is exactly what trips upstream's dangling
+`pw_buffer->user_data`. The signature to recognise:
+
+```
+punktfunk-gamescope: ../src/pipewire.cpp:88: void destroy_buffer(pipewire_buffer*):
+  Assertion `false' failed.
+#5  destroy_buffer(pipewire_buffer*).cold
+```
+
+It is a use-after-free wearing an `assert(false); // unreachable` as a disguise — `buffer->type`
+is read out of freed memory and falls off the end of the `switch`. A zeroed slot gives the SIGSEGV
+variant of the same fault instead. Two traps when triaging it:
+
+* **It is not HDR-specific.** The abort was first seen right after a 10-bit stream negotiated, so
+  it looked like the HDR path and `PUNKTFUNK_GAMESCOPE_HDR=0` looked like a workaround. It is not
+  — the same abort reproduces on an SDR session with no `--hdr-enabled` in the command line.
+  Check the failing process's actual argv before believing an HDR association.
+* **`gamescope-session-plus` hides it.** When our binary crash-loops, the session script retries
+  and eventually comes up on the *stock* `/usr/bin/gamescope` at its default 1920×1080. So the box
+  lands in a working-looking game mode at the wrong resolution and without any of these patches.
+  Read the banner in `~/.gamescope-stdout.log`, not the fact that a session exists.
+
 ## Why the marker exists
 
 punktfunk decides a session's shape **before** the virtual display exists: the bit depth at
@@ -130,6 +157,7 @@ They are gamescope's, not ours, and they vary by distro. Two shortcuts that work
 sudo dnf install -y dnf-plugins-core meson ninja-build glslc
 sudo dnf builddep -y gamescope
 sudo dnf install -y xorg-x11-server-Xwayland-devel      # NOT pulled by builddep; wlroots needs it
+sudo dnf install -y libstdc++-static                    # NOT pulled by builddep; see below
 
 # Arch / SteamOS — see the makedepends in ./PKGBUILD
 ```
@@ -140,6 +168,19 @@ we pin, so it can come up short. `xorg-x11-server-Xwayland-devel` is the one tha
 nor a xserver.wrap file was found`, several minutes into an otherwise clean run. If a different
 one surfaces, meson names it — install and re-run with `--srcdir` so the clone is not repeated.
 
+⚠️ `libstdc++-static` is a **punktfunk** requirement, not gamescope's, so no builddep will ever pull
+it: the build script links the C++ runtime statically on purpose (see the long comment beside
+`LDFLAGS` in `build-punktfunk-gamescope.sh`). Without it meson fails at configure with a message
+that names neither the flag nor the package (2026-08-10, Fedora 44):
+
+```
+ERROR: Compiler c++ cannot compile programs.
+  /usr/bin/ld.bfd: cannot find -lstdc++
+```
+
+The cleanup trap deletes the temp checkout on failure, taking `meson-logs/meson-log.txt` with it —
+so build with `--srcdir` when diagnosing, or the evidence is gone before you can read it.
+
 `gamescope` needs `CAP_SYS_NICE` for its realtime priority; the distro packages set it on their
 own binary. Mirror it if you install ours system-wide:
 

packaging/gamescope/patches/0005-punktfunk-stamp-the-version-banner-with-pfhdrN.patch

@@ -17,18 +17,19 @@ The number is a monotonic patch-set revision, so one probe answers all of them:
   +pfhdr2  …and --pipewire-composite-cursor
   +pfhdr3  …and the headless connector advertises its mode + --custom-refresh-rates
   +pfhdr4  …and --pipewire-composite-external-overlay
+  +pfhdr5  …and the PipeWire buffer use-after-free is fixed (no new capability)
 
 NOT for upstream: drop this once the functional patches land there and plain
 version floors answer the same questions.
 ---
- src/meson.build | 9 ++++++++-
- 1 file changed, 8 insertions(+), 1 deletion(-)
+ src/meson.build | 10 +++++++++-
+ 1 file changed, 9 insertions(+), 1 deletion(-)
 
 diff --git a/src/meson.build b/src/meson.build
 index 662f752..12fd38a 100644
 --- a/src/meson.build
 +++ b/src/meson.build
-@@ -177,7 +177,14 @@ compiler_version = cc.version()
+@@ -177,7 +177,15 @@ compiler_version = cc.version()
  
  vcs_tag_cmd = ['git', 'describe', '--always', '--tags', '--dirty=+']
  vcs_tag = run_command(vcs_tag_cmd, check: false).stdout().strip()
@@ -40,7 +41,8 @@ index 662f752..12fd38a 100644
 +#   +pfhdr2 — …and `--pipewire-composite-cursor`
 +#   +pfhdr3 — …and the headless connector advertises its mode + `--custom-refresh-rates`
 +#   +pfhdr4 — …and `--pipewire-composite-external-overlay`
-+version_tag = vcs_tag + '+pfhdr4' + ' (' + compiler_name + ' ' + compiler_version + ')'
++#   +pfhdr5 — …and the PipeWire buffer use-after-free is fixed (no new capability)
++version_tag = vcs_tag + '+pfhdr5' + ' (' + compiler_name + ' ' + compiler_version + ')'
  
  gamescope_version_conf = configuration_data()
  gamescope_version_conf.set('VCS_TAG', version_tag)

packaging/gamescope/patches/0007-pipewire-never-leave-pw_buffer-user_data-pointing-at.patch

@@ -0,0 +1,132 @@
+From d3a1c36bf871aaaa4ad9c2baf7170bba69742189 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Enrico=20B=C3=BChler?= <enrico.buehler@unom.io>
+Date: Mon, 10 Aug 2026 01:55:00 +0200
+Subject: [PATCH] pipewire: never leave pw_buffer->user_data pointing at a
+ freed buffer
+
+A gamescope session driving a PipeWire capture consumer dies as soon as the
+stream renegotiates:
+
+  gamescope: ../src/pipewire.cpp:88: void destroy_buffer(pipewire_buffer*):
+    Assertion `false' failed.
+  #4  __assert_fail
+  #5  destroy_buffer(pipewire_buffer*).cold
+  #13 destroy_buffer(pipewire_buffer*)
+
+with a SIGSEGV variant of the same fault when the slot happens to be zeroed
+instead of stale. On a streaming host this reproduces on essentially every
+client connect, because the host sets the session to the client's mode and the
+mode change is what renegotiates the stream.
+
+pw_buffer->user_data and the pipewire_buffer behind it are associated in
+exactly one place - the bottom of stream_handle_add_buffer - but that
+assignment sits AFTER all four `goto error` paths, and the `error:` label is a
+bare `delete buffer`. Any failing add therefore leaves the slot holding either
+nothing or the previous generation's freed pointer.
+
+The teardown path leaks the same association even when nothing fails.
+stream_handle_remove_buffer clears buffer->buffer, which is the only route from
+the pipewire_buffer back to its pw_buffer. If the buffer is still `copying` it
+is not deleted here at all - the steamcompmgr thread deletes it later, by which
+point the pw_buffer can no longer be reached to be cleared. The slot is then a
+dangling pointer for the rest of its life.
+
+PipeWire recycles struct pw_buffer slots across renegotiations, so the next
+remove_buffer on a recycled slot dereferences freed memory. buffer->type reads
+as garbage, falls off the end of the switch, and hits the default:
+assert(false). The assert(buffer->buffer == nullptr) at the top passes on the
+way in, which is what makes this look like an "unreachable" assert rather than
+the use-after-free it is.
+
+Fix the association rather than the symptom:
+
+  - set pw_buffer->user_data immediately after the allocation, so it is valid
+    for every path out of add_buffer, and clear it on the error path;
+  - clear it in remove_buffer, the last moment at which both halves are still
+    known, whether or not the buffer is deleted there;
+  - null-check the two places that consume it.
+
+With user_data non-null iff a live pipewire_buffer exists for that slot, the
+default: case is genuinely unreachable - so it logs and returns instead of
+aborting, since losing a capture buffer should not take the session down.
+
+Upstream: yes - a plain use-after-free in the PipeWire buffer lifecycle, with
+nothing punktfunk-specific about it.
+---
+ src/pipewire.cpp | 33 +++++++++++++++++++++++++++++----
+ 1 file changed, 29 insertions(+), 4 deletions(-)
+
+diff --git a/src/pipewire.cpp b/src/pipewire.cpp
+index 76b3ea8..faecf04 100644
+--- a/src/pipewire.cpp
++++ b/src/pipewire.cpp
+@@ -51,8 +51,13 @@ static void destroy_buffer(struct pipewire_buffer *buffer) {
+ 	case SPA_DATA_DmaBuf:
+ 		break; // nothing to do
+ 	default:
+-		assert(false); // unreachable
+-	}	
++		// A buffer that never reached the point of choosing a type owns nothing to
++		// release. The association fixes above should make this unreachable, but a
++		// capture stream is not worth aborting a whole session over.
++		pwr_log.errorf("destroy_buffer: buffer has no data type (%d); nothing to release",
++			(int) buffer->type);
++		break;
++	}
+ 
+ 	// If out_buffer == buffer, then set it to nullptr.
+ 	// We don't care about the result.
+@@ -179,6 +184,13 @@ static void request_buffer(struct pipewire_state *state)
+ 	}
+ 
+ 	struct pipewire_buffer *buffer = (struct pipewire_buffer *) pw_buffer->user_data;
++	if (buffer == nullptr) {
++		// add_buffer bailed for this slot, so it carries no state. Hand it back
++		// rather than dereferencing nothing.
++		pwr_log.errorf("dequeued a buffer with no state; requeuing");
++		pw_stream_queue_buffer(state->stream, pw_buffer);
++		return;
++	}
+ 	buffer->copying = true;
+ 
+ 	// Past this exchange, the PipeWire thread shares the buffer with the
+@@ -464,6 +476,10 @@ static void stream_handle_add_buffer(void *user_data, struct pw_buffer *pw_buffe
+ 
+ 	struct pipewire_buffer *buffer = new pipewire_buffer();
+ 	buffer->buffer = pw_buffer;
++	// Associate the two halves NOW, not after the last `goto error` below: every error
++	// path deletes this buffer, and a slot left holding a stale pointer is one the next
++	// remove_buffer dereferences.
++	pw_buffer->user_data = buffer;
+ 	buffer->video_info = state->video_info;
+ 	buffer->gamescope_info = state->gamescope_info;
+ 
+@@ -583,17 +599,26 @@ static void stream_handle_add_buffer(void *user_data, struct pw_buffer *pw_buffe
+ 		goto error;
+ 	}
+ 
+-	pw_buffer->user_data = buffer;
+-
+ 	return;
+ 
+ error:
++	pw_buffer->user_data = nullptr;
+ 	delete buffer;
+ }
+ 
+ static void stream_handle_remove_buffer(void *data, struct pw_buffer *pw_buffer)
+ {
+ 	struct pipewire_buffer *buffer = (struct pipewire_buffer *) pw_buffer->user_data;
++	if (buffer == nullptr) {
++		return;
++	}
++
++	// The LAST point at which both halves of the association are still known. Clearing
++	// buffer->buffer below makes the pw_buffer unreachable from the pipewire_buffer, and
++	// a buffer that is still `copying` is deleted later, on the steamcompmgr thread —
++	// with no way to reach back here. Sever the link now or the slot is left pointing at
++	// memory that is about to be freed.
++	pw_buffer->user_data = nullptr;
+ 
+ 	buffer->buffer = nullptr;
+