From cf77ee87a1586fe4213e2690ffcdfdfdebc1790c Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Tue, 21 Jul 2026 00:31:12 -0300 Subject: [PATCH] drm: second review pass on the phase-2 split 1- make PipeWire init atomic: build every per-display capturer into owned staging first and publish them to CAP_DISPLAY_INFO only after all succeed, so a mid-loop Capturer::new failure neither leaves partial entries (which the next check_init would treat as already-initialized) nor leaks the raw pointers already created. 2- pin the immutable libdrmtap commit, not just the tag: git clone --branch follows a mutable tag, so verify the cloned HEAD equals DRMTAP_SHA in both the CI workflow and build.py, failing on a moved/compromised tag. 3- drop the stale comment claiming a libdrmtap-sys crate pin (the drm backend has no such dependency). --- .github/workflows/flutter-build.yml | 15 ++++++++++++--- build.py | 11 +++++++++++ src/server/wayland.rs | 29 ++++++++++++++++------------- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/.github/workflows/flutter-build.yml b/.github/workflows/flutter-build.yml index 4f5f80adc..fa6e60674 100644 --- a/.github/workflows/flutter-build.yml +++ b/.github/workflows/flutter-build.yml @@ -1702,11 +1702,13 @@ jobs: # exported so build.py reuses the exact same source. We clone + build # the .so here and hand it to build.py via DRMTAP_PREBUILT_DIR, because # a later build step in this container disturbs the working tree. - # DRMTAP_REF is an EXACT release tag (vX.Y.Z), NOT a branch: the bundled - # .so version must match the `libdrmtap-sys = "=0.4.13"` crate pin in - # libs/scrap/Cargo.toml (the loader only checks ABI-major). Bump both together. + # DRMTAP_REF is an EXACT release tag (vX.Y.Z), NOT a branch, and it is verified + # against the pinned immutable commit DRMTAP_SHA after clone (below). The drm backend + # has NO libdrmtap-sys Cargo dependency: rustdesk dlopens this .so at runtime + # (drmtap_dl.rs checks ABI-major). Keep DRMTAP_SHA in sync with the tag on every bump. export DRMTAP_REPO="https://github.com/rustdesk-org/libdrmtap" export DRMTAP_REF="v0.4.13" + export DRMTAP_SHA="${DRMTAP_SHA:-c9cf0938f3b10a3d4a9eeb9c6f97aaa1606c6b4a}" # Guard: refuse a loose/branch ref so a moving `main` can never silently # regress the pin. Only an EXACT vX.Y.Z tag is accepted (a strict anchored # match, so values like v0.4.13-ci or a branch that resolves under @@ -1718,6 +1720,13 @@ jobs: rm -rf third_party/libdrmtap git clone --depth 1 --branch "$DRMTAP_REF" "$DRMTAP_REPO" third_party/libdrmtap test -f third_party/libdrmtap/meson.build || { echo "FATAL: libdrmtap source missing"; exit 1; } + # Pin the immutable commit, not only the tag name: `git clone --branch` follows a + # mutable tag, so a moved or compromised v0.4.13 could swap the root-loaded .so while + # the regex above still passes. Verify the cloned HEAD is exactly the pinned SHA. + got_sha="$(git -C third_party/libdrmtap rev-parse HEAD)" + if [ "$got_sha" != "$DRMTAP_SHA" ]; then + echo "FATAL: libdrmtap $DRMTAP_REF resolved to $got_sha, expected $DRMTAP_SHA (moved/compromised tag?)"; exit 1 + fi # Build libdrmtap.so now, while the cloned source is definitely # present, and stash the real object OUTSIDE the source tree. A later # build step in this container disturbs that working tree (it ends up diff --git a/build.py b/build.py index 048996d41..17eb57913 100755 --- a/build.py +++ b/build.py @@ -8,6 +8,7 @@ import zipfile import urllib.request import shutil import hashlib +import subprocess import argparse import sys from pathlib import Path @@ -342,6 +343,10 @@ def ffi_bindgen_function_refactor(): # local testing or another fork. LIBDRMTAP_REPO = os.environ.get('DRMTAP_REPO', 'https://github.com/rustdesk-org/libdrmtap') LIBDRMTAP_REF = os.environ.get('DRMTAP_REF', 'v0.4.13') +# The immutable commit the release tag must resolve to. `git clone --branch` follows a mutable tag, +# so verifying this after clone catches a moved/compromised tag swapping the .so. Keep in sync with +# LIBDRMTAP_REF on every bump (override via DRMTAP_SHA together with DRMTAP_REF for a local fork). +LIBDRMTAP_SHA = os.environ.get('DRMTAP_SHA', 'c9cf0938f3b10a3d4a9eeb9c6f97aaa1606c6b4a') def _single_real_so(paths, where): @@ -378,6 +383,12 @@ def build_libdrmtap_so(): shutil.rmtree(src) os.makedirs(os.path.dirname(src), exist_ok=True) system2(f'git clone --depth 1 --branch {LIBDRMTAP_REF} {LIBDRMTAP_REPO} {src}') + got_sha = subprocess.check_output( + ['git', '-C', src, 'rev-parse', 'HEAD']).decode().strip() + if got_sha != LIBDRMTAP_SHA: + raise Exception( + f'libdrmtap {LIBDRMTAP_REF} resolved to {got_sha}, expected {LIBDRMTAP_SHA} ' + f'(moved/compromised tag?)') build_dir = os.path.join(src, 'build-pkg') if not os.path.exists(os.path.join(build_dir, 'build.ninja')): system2(f'meson setup {build_dir} {src} --buildtype=release') diff --git a/src/server/wayland.rs b/src/server/wayland.rs index f2fe74e8d..748ffaaa9 100644 --- a/src/server/wayland.rs +++ b/src/server/wayland.rs @@ -249,14 +249,20 @@ pub(super) async fn check_init() -> ResultType<()> { num_cpus::get() ); - // Create individual CapDisplayInfo for each display with its own capturer + // Create every per-display capturer FIRST into owned temporary storage, so a failure + // partway does not publish a partial set. If `Capturer::new` errors, the `?` returns + // and the already-staged capturers drop cleanly (no leaked raw pointers, and + // CAP_DISPLAY_INFO stays empty so the next check_init retries instead of seeing a + // non-empty map and skipping re-init). + let mut staged = Vec::with_capacity(num); for (idx, display) in all.into_iter().enumerate() { - let capturer = - Box::into_raw(Box::new(Capturer::new(display).with_context(|| { - format!("Failed to create capturer for display {}", idx) - })?)); - let capturer = CapturerPtr(capturer); - + let capturer = Capturer::new(display) + .with_context(|| format!("Failed to create capturer for display {}", idx))?; + staged.push((idx, capturer)); + } + // All capturers created: publish them atomically. + for (idx, capturer) in staged { + let capturer = CapturerPtr(Box::into_raw(Box::new(capturer))); let cap_display_info = Box::into_raw(Box::new(CapDisplayInfo { rects: rects.clone(), displays: displays.clone(), @@ -265,14 +271,11 @@ pub(super) async fn check_init() -> ResultType<()> { current: idx, capturer, })); - lock.insert(idx, cap_display_info as u64); } - // Mark PipeWire initialized only AFTER every per-display capturer was created and - // stored. Setting it earlier meant a partial failure above (a `Capturer::new` error - // propagated by `?`) returned Err with the flag already true, so the next check_init - // saw "initialized", skipped re-init, and left CAP_DISPLAY_INFO empty (no capture). - // This matters more now that the per-display DRM->PipeWire fallback funnels through here. + // Mark PipeWire initialized only AFTER the full set was published, so a partial + // failure above leaves the flag false and the next check_init retries. This matters + // more now that the per-display DRM->PipeWire fallback funnels through here. *PIPEWIRE_INITIALIZED.write().unwrap() = true; } }