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).
This commit is contained in:
Mariano Abad
2026-07-21 00:31:12 -03:00
parent 6cc426e9f5
commit cf77ee87a1
3 changed files with 39 additions and 16 deletions

View File

@@ -1702,11 +1702,13 @@ jobs:
# exported so build.py reuses the exact same source. We clone + build # 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 # 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. # 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 # DRMTAP_REF is an EXACT release tag (vX.Y.Z), NOT a branch, and it is verified
# .so version must match the `libdrmtap-sys = "=0.4.13"` crate pin in # against the pinned immutable commit DRMTAP_SHA after clone (below). The drm backend
# libs/scrap/Cargo.toml (the loader only checks ABI-major). Bump both together. # 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_REPO="https://github.com/rustdesk-org/libdrmtap"
export DRMTAP_REF="v0.4.13" 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 # 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 # 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 # match, so values like v0.4.13-ci or a branch that resolves under
@@ -1718,6 +1720,13 @@ jobs:
rm -rf third_party/libdrmtap rm -rf third_party/libdrmtap
git clone --depth 1 --branch "$DRMTAP_REF" "$DRMTAP_REPO" 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; } 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 # Build libdrmtap.so now, while the cloned source is definitely
# present, and stash the real object OUTSIDE the source tree. A later # present, and stash the real object OUTSIDE the source tree. A later
# build step in this container disturbs that working tree (it ends up # build step in this container disturbs that working tree (it ends up

View File

@@ -8,6 +8,7 @@ import zipfile
import urllib.request import urllib.request
import shutil import shutil
import hashlib import hashlib
import subprocess
import argparse import argparse
import sys import sys
from pathlib import Path from pathlib import Path
@@ -342,6 +343,10 @@ def ffi_bindgen_function_refactor():
# local testing or another fork. # local testing or another fork.
LIBDRMTAP_REPO = os.environ.get('DRMTAP_REPO', 'https://github.com/rustdesk-org/libdrmtap') LIBDRMTAP_REPO = os.environ.get('DRMTAP_REPO', 'https://github.com/rustdesk-org/libdrmtap')
LIBDRMTAP_REF = os.environ.get('DRMTAP_REF', 'v0.4.13') 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): def _single_real_so(paths, where):
@@ -378,6 +383,12 @@ def build_libdrmtap_so():
shutil.rmtree(src) shutil.rmtree(src)
os.makedirs(os.path.dirname(src), exist_ok=True) os.makedirs(os.path.dirname(src), exist_ok=True)
system2(f'git clone --depth 1 --branch {LIBDRMTAP_REF} {LIBDRMTAP_REPO} {src}') 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') build_dir = os.path.join(src, 'build-pkg')
if not os.path.exists(os.path.join(build_dir, 'build.ninja')): if not os.path.exists(os.path.join(build_dir, 'build.ninja')):
system2(f'meson setup {build_dir} {src} --buildtype=release') system2(f'meson setup {build_dir} {src} --buildtype=release')

View File

@@ -249,14 +249,20 @@ pub(super) async fn check_init() -> ResultType<()> {
num_cpus::get() 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() { for (idx, display) in all.into_iter().enumerate() {
let capturer = let capturer = Capturer::new(display)
Box::into_raw(Box::new(Capturer::new(display).with_context(|| { .with_context(|| format!("Failed to create capturer for display {}", idx))?;
format!("Failed to create capturer for display {}", idx) staged.push((idx, capturer));
})?)); }
let capturer = CapturerPtr(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 { let cap_display_info = Box::into_raw(Box::new(CapDisplayInfo {
rects: rects.clone(), rects: rects.clone(),
displays: displays.clone(), displays: displays.clone(),
@@ -265,14 +271,11 @@ pub(super) async fn check_init() -> ResultType<()> {
current: idx, current: idx,
capturer, capturer,
})); }));
lock.insert(idx, cap_display_info as u64); lock.insert(idx, cap_display_info as u64);
} }
// Mark PipeWire initialized only AFTER every per-display capturer was created and // Mark PipeWire initialized only AFTER the full set was published, so a partial
// stored. Setting it earlier meant a partial failure above (a `Capturer::new` error // failure above leaves the flag false and the next check_init retries. This matters
// propagated by `?`) returned Err with the flag already true, so the next check_init // more now that the per-display DRM->PipeWire fallback funnels through here.
// 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.
*PIPEWIRE_INITIALIZED.write().unwrap() = true; *PIPEWIRE_INITIALIZED.write().unwrap() = true;
} }
} }