From 78bfe615547aaa8f431c66b49404d4106582f3da Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Thu, 30 Jul 2026 16:57:09 -0300 Subject: [PATCH] drm: close the round-14 review findings - the .so contract and deb assertions no longer pipe into grep. under `set -o pipefail`, `producer | grep -q` reports a FALSE FAILURE once the producer outruns the 64 KB pipe buffer: grep -q exits at the first match, the producer dies on SIGPIPE, and pipefail makes that the pipeline's status - so a library that HAS the symbol is reported as missing it and the step fails on a good build. measured on a real EGL-enabled .so (101 KB of strings, both markers present): the piped form reported both missing. this was introduced by the strictness fix two rounds ago and only passes today because a release-sized .so fits in the buffer. NOTE the obvious repair does not work either - materializing the output and piping the variable keeps the pipe and fails identically (measured), so these now match with bash's own pattern operator and no subprocess at all. verified with positive and negative controls. - warm_availability decides X11 for itself, inside its retry loop, with the UNMEMOISED `scrap::is_x11()`. this is the same one-shot-at -startup bug the pre-warm had, in its sibling call site, left behind when that one was fixed: the check ran during startup, where loginctl cannot yet name the seat0 session and the answer defaults to "x11", so a Wayland host that came up slowly skipped the warm for the life of the process and got back the cold-probe "No displays" symptom the warm exists to remove. the memoised form would have moved the bug rather than fixed it, since it latches its first answer. - the grab_desc SAFETY comment says what the frame protocol actually is instead of promising a release on every return path: traced in the C, a failing grab_desc leaves nothing to release (-EINVAL returns before allocating, a failed inner grab has already cleaned up, and -ENOTSUP releases the frame itself), so releasing on those paths would be a double free. --- .github/workflows/drm-capture.yml | 33 +++++++++++++++++++++++++---- libs/scrap/src/common/drm_reader.rs | 10 +++++++-- src/server.rs | 13 +++++++----- src/server/drm_capturer.rs | 24 ++++++++++++++++++--- 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index c058770e6..d9b610d57 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -215,8 +215,22 @@ jobs: echo "::error::extracted only $nsyms loader symbols from drmtap_dl.rs (expected >= 13); the extraction pattern no longer matches the loader" missing=1 fi + # Inspect the object ONCE into a variable, then match with bash's own pattern operator -- + # NO PIPE ANYWHERE IN THESE CHECKS. `anything | grep -q` under `set -o pipefail` reports a + # FALSE FAILURE as soon as the producer outruns the 64 KB pipe buffer: grep -q exits at the + # first match, the producer dies on SIGPIPE (141), and pipefail makes that the pipeline's + # status, so a library that HAS the symbol is reported as missing it. Measured on a real + # EGL-enabled .so (101 KB of `strings`, both markers present): the piped form reported both + # missing and failed the step. Note the obvious repair does NOT work -- materializing the + # output and then doing `printf '%s\n' "$var" | grep -q` keeps the pipe and just swaps the + # producer, and it fails identically (measured). Today's release-sized .so happens to fit in + # the buffer, which is the only reason this has not fired yet. + exported="$(nm -D --defined-only "$SO")" + strs="$(strings "$SO")" for sym in $syms; do - if ! nm -D --defined-only "$SO" | grep -q " T $sym\$"; then + # Line-anchored: wrap in newlines so the pattern can require a whole line, the same + # thing `grep " T $sym$"` was expressing. + if [[ $'\n'"$exported"$'\n' != *$'\n'*" T $sym"$'\n'* ]]; then echo "::error::libdrmtap does not export $sym, which the runtime loader resolves" missing=1 fi @@ -226,7 +240,7 @@ jobs: # for: the naive ELF check reports "no EGL" on a perfectly good library. What a CPU-only stub # build really lacks is the dlopen target name and the import call itself. for s in "libEGL.so.1" "eglCreateImageKHR"; do - if ! strings "$SO" | grep -qF "$s"; then + if [[ "$strs" != *"$s"* ]]; then echo "::error::libdrmtap looks like a CPU-only stub (no $s): the EGL detile hot path is missing" missing=1 fi @@ -346,8 +360,19 @@ jobs: fi deb="${debs[0]}" echo "::notice::built $deb ($(stat -c %s "$deb") bytes)" - dpkg -c "$deb" | grep -E 'usr/lib/rustdesk/libdrmtap\.so\.0\.[0-9]+\.[0-9]+$' - dpkg -c "$deb" | grep -E 'usr/lib/rustdesk/libdrmtap\.so\.0 ->' + # Pipe-free for the same reason as the .so contract step above (see the comment there: + # a producer feeding a grep that can exit early is a SIGPIPE reported as a failure under + # pipefail). `grep -E` without -q reads to EOF so these two happen to be safe, but the + # shape is the hazard and the next `-q` added here would inherit it silently. + contents="$(dpkg -c "$deb")" + if [[ ! "$contents" =~ usr/lib/rustdesk/libdrmtap\.so\.0\.[0-9]+\.[0-9]+ ]]; then + echo "::error::the deb does not contain a versioned libdrmtap.so.0.x.y" + exit 1 + fi + if [[ "$contents" != *"usr/lib/rustdesk/libdrmtap.so.0 ->"* ]]; then + echo "::error::the deb does not contain the libdrmtap.so.0 soname symlink" + exit 1 + fi # The library alone does not make this a drm build: build.py stages it whenever --drm is # passed, independently of what was compiled, and the deb name is what tells a user this # is the consent-bypass variant. Assert the BINARY too, by the absolute dlopen path that diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 4f9e5bf0b..65863976d 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -365,8 +365,14 @@ impl DrmReader { /// libdrmtap that does not export this symbol (see `drmtap_dl::abi_accepted`). pub fn grab_desc(&mut self) -> io::Result<(OwnedFd, drmtap_dmabuf_desc)> { let grab_desc = self.lib.grab_desc; - // SAFETY: self.ctx is a valid context; desc/frame are zeroed before the - // call and the frame is released on every return path (after the dup). + // SAFETY: self.ctx is a valid context; desc/frame are zeroed before the call. The frame is + // released on every return path that OWNS one, which is not the same as every return path: + // a failing `drmtap_grab_desc` leaves nothing for us to release, and releasing anyway would + // be a double free. Traced in the C rather than assumed -- on `-EINVAL` it returns before + // allocating, on a failed inner grab that grab has already cleaned up after itself, and on + // `-ENOTSUP` (pixels but no transferable fd) libdrmtap calls `drmtap_frame_release` ITSELF + // before returning. So the error arm below deliberately returns without releasing, and only + // the paths that reach a populated frame release it, after dupping the fd out of it. unsafe { let mut desc: drmtap_dmabuf_desc = std::mem::zeroed(); let mut frame: drmtap_frame_info = std::mem::zeroed(); diff --git a/src/server.rs b/src/server.rs index be62e74d0..4029ed453 100644 --- a/src/server.rs +++ b/src/server.rs @@ -603,12 +603,15 @@ pub async fn start_server(is_server: bool, no_server: bool) { }); // Warm the DRM availability cache before any client connects, so the first connection does // not race a cold `_drm` probe and ship an empty display list ("No displays" + retry). - // Skipped on X11: every consumer of the verdict is behind an `!is_x11()` check, so probing - // there makes the root service open DRM readers for a path this session can never use. + // X11 is skipped -- probing there makes the root service open DRM readers for a path this + // session can never take -- but that decision belongs to `warm_availability`, which already + // makes it, and NOT to this call site. Deciding it here is the same one-shot-at-startup + // mistake the pre-warm had: `is_x11()` answers "x11" whenever loginctl cannot yet name the + // seat0 session, which during a boot is exactly when this runs, and nothing revisits it -- + // so a Wayland host that came up slowly skipped the warm for the life of the process and + // got back the cold-probe "No displays" symptom the warm exists to remove. #[cfg(all(target_os = "linux", feature = "drm"))] - if !scrap::is_x11() { - std::thread::spawn(drm_capturer::warm_availability); - } + std::thread::spawn(drm_capturer::warm_availability); input_service::fix_key_down_timeout_loop(); #[cfg(target_os = "linux")] if input_service::wayland_use_uinput() { diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 81f266de2..1ba913e59 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -1411,10 +1411,28 @@ pub(super) fn warm_availability() { // Nothing on X11 can consume a DRM stream, and probing makes the ROOT service open DRM readers, // so an X11 host running a drm build would pay that at every startup for a path it can never // take. The lazy probe behind is_available is reached only from the Wayland paths already. - if crate::platform::linux::is_x11() { - return; - } + // + // Two things about WHERE and HOW this is decided, both learned the hard way on the pre-warm's + // identical gate (see `drm_prewarm` in ipc/drm.rs): + // + // - it is decided HERE, not at the call site in server.rs. This runs during startup, and + // `get_display_server()` answers "x11" whenever loginctl cannot yet name the seat0 session, + // which during a boot is precisely then. A one-shot check outside the retry loop skipped the + // warm for the life of the process on a Wayland host that came up slowly, handing back the + // cold-probe "No displays" symptom this function exists to remove. + // - it uses `scrap::is_x11()`, the UNMEMOISED form that re-runs loginctl per call. + // `crate::platform::linux::is_x11()` latches its first answer in a lazy_static, so asking it + // inside a retry loop would re-read the same early "x11" ten times and change nothing. + // + // The re-check rides the existing retry loop rather than adding a second wait: a genuine X11 + // host spends the same ten short attempts it already spent on `query_displays` and opens + // nothing, and a host whose session turns out to be Wayland proceeds on the attempt where that + // becomes knowable. for _ in 0..10 { + if scrap::is_x11() { + std::thread::sleep(Duration::from_millis(300)); + continue; + } if matches!(&*DRM_STATE.lock().unwrap(), ProbeState::Available(..)) { return; }