From f524d41279ddae6ad85d1a2e25e6037d2d8aed3e Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Thu, 30 Jul 2026 14:03:26 -0300 Subject: [PATCH] drm: close the round-10 review findings - the /dev/dri gate returns the CANONICAL path instead of a bool, and both callers open that value. answering yes/no meant the caller handed the original string to libdrmtap, which re-resolved every symlink component after the check - a check-then-use window, in the root service. this is the whole point of the gate, so it should never have been able to hand back an unresolved path. - `--package --drm` builds the capture library instead of demanding it inside the bundle. no build path puts libdrmtap in a bundle folder (the flutter deb builds it straight into the staged deb), so that check made the flag combination impossible to satisfy. the safety property it stood in for is now asserted directly and better: the staged BINARY must carry the drm dlopen path, so a stock binary can never be packaged under the consent-bypass name. a bundle that does carry a .so keeps its existing EGL assertion, and the variant naming keys on the explicit request rather than on what happened to be staged. - the deb assert step globs into an array and asserts the count: under set -e `ls` aborted before its own `test -n` could report, and several matches produced a multi-line value whose mv failed with an unrelated error. --- .github/workflows/drm-capture.yml | 12 ++++- build.py | 75 ++++++++++++++++++----------- libs/scrap/src/common/drm_reader.rs | 35 +++++++++----- libs/scrap/src/common/drm_render.rs | 15 +++--- 4 files changed, 87 insertions(+), 50 deletions(-) diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index c574db3cb..ce07ba0f2 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -323,8 +323,16 @@ jobs: # Strict mode so the mid-script checks can fail the step (without it only the LAST # command's status counts and the greps above it are decorative). set -euo pipefail - deb="$(ls rustdesk-unattended-wayland-*.deb)" - test -n "$deb" + # Glob into an array and assert the COUNT. `deb="$(ls ...)"` aborted on zero matches + # before its own `test -n` could report, and on several matches produced a multi-line + # value whose `mv` failed with something unrelated to the real problem. + shopt -s nullglob + debs=(rustdesk-unattended-wayland-*.deb) + if [ "${#debs[@]}" -ne 1 ]; then + echo "::error::expected exactly one rustdesk-unattended-wayland-*.deb, found ${#debs[@]}: ${debs[*]-none}" + exit 1 + 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 ->' diff --git a/build.py b/build.py index 27c493b1c..928784b49 100755 --- a/build.py +++ b/build.py @@ -628,41 +628,60 @@ def build_deb_from_folder(version, binary_folder, want_drm=False): 'cp ../res/rustdesk-link.desktop tmpdeb/usr/share/applications/rustdesk-link.desktop') system2( "echo \"#!/bin/sh\" >> tmpdeb/usr/share/rustdesk/files/polkit && chmod a+x tmpdeb/usr/share/rustdesk/files/polkit") - # A staged bundle (binary_folder) carries its own libdrmtap.so.0* for a --drm build, so we do - # not rebuild it here; the `cp -r` above placed it under usr/share/rustdesk/. Move it to the - # private lib dir, then finalize the deb the same way build_flutter_deb does. + # Where the capture library comes from for a `--package --drm` build. Two shapes are + # supported, because two exist in practice: a bundle that already carries libdrmtap.so.0.* + # (someone staged it, e.g. a CI artifact), and a plain bundle, which is what every build path + # here actually produces -- the flutter deb builds the library straight into the staged deb, so + # nothing ever puts it inside the bundle folder. Demanding it in the bundle made this flag + # combination impossible to satisfy. bundled_glob = glob.glob('tmpdeb/usr/share/rustdesk/libdrmtap.so.0.*') - ships_so = any(os.path.isfile(p) and not os.path.islink(p) for p in bundled_glob) - # The variant must be decided by the EXPLICIT --drm request, not merely by what happens - # to be staged. Cross-check the two and fail loudly on a mismatch: a drm binary staged - # WITHOUT its libdrmtap.so.0.* would otherwise be silently shipped as the stock - # `rustdesk` package (no drm deps, a dlopen that can never resolve), and a - # bundle that DOES carry the .so would be shipped as the consent-bypass variant even - # when --drm was never asked for. - if want_drm and not ships_so: - raise Exception( - '--drm was requested but no real libdrmtap.so.0.* is staged under ' - 'usr/share/rustdesk/ in the bundle; refusing to package a drm binary as the ' - 'stock rustdesk package (it would ship without the capture library or its deps)') - if ships_so and not want_drm: + bundle_carries_so = any(os.path.isfile(p) and not os.path.islink(p) for p in bundled_glob) + # The variant must be decided by the EXPLICIT --drm request, not merely by what happens to be + # staged: a bundle that carries the .so must NOT be shipped as the consent-bypass variant when + # --drm was never passed. + if bundle_carries_so and not want_drm: raise Exception( 'the staged bundle carries libdrmtap.so.0.* but --drm was not passed; refusing ' 'to silently ship the consent-bypass unattended-wayland variant (pass --drm to ' 'build it deliberately)') - if ships_so: - so = _single_real_so(bundled_glob, 'the staged --drm bundle') - # The THIRD artifact source, and the last one that was missing the check: --package takes the - # .so straight out of a bundle somebody else produced, so it has the same exposure as - # DRMTAP_PREBUILT_DIR (see the comment on that branch). A CPU-only stub would ship, the loader - # would accept it, and capture would degrade to PipeWire without a word. - _assert_so_has_egl(so) - stage_libdrmtap_into_deb(so) - system2(f'rm -f {so}') - system2('rm -f tmpdeb/usr/share/rustdesk/libdrmtap.so tmpdeb/usr/share/rustdesk/libdrmtap.so.0') + if want_drm: + # Whichever shape we are in, the staged BINARY must really be a drm build. This is the + # property the old presence-of-the-.so test stood in for, badly: a stock binary packaged as + # the unattended-wayland variant would carry the consent-bypass name, conflict with and + # replace the stock package, and never be able to capture. The marker is the absolute + # dlopen path from drmtap_dl.rs, present only when the feature is compiled in -- the same + # kind of artifact assertion as _assert_so_has_egl, and for the same reason: assert what + # was produced, not what was asked for. + marker = b'/usr/lib/rustdesk/libdrmtap.so.0' + binaries = [p for p in glob.glob('tmpdeb/usr/share/rustdesk/lib/librustdesk.so') + + glob.glob('tmpdeb/usr/share/rustdesk/rustdesk') if os.path.isfile(p)] + if not any(marker in open(p, 'rb').read() for p in binaries): + raise Exception( + f'--drm was requested but the staged bundle does not look like a drm build (no ' + f'{marker.decode()} dlopen path in {binaries or "any staged binary"}); refusing to ' + 'package it as the unattended-wayland variant, which conflicts with and replaces ' + 'the stock package but could never capture') + if bundle_carries_so: + so = _single_real_so(bundled_glob, 'the staged --drm bundle') + # The THIRD artifact source, and the last one that was missing the check: --package + # takes the .so straight out of a bundle somebody else produced, so it has the same + # exposure as DRMTAP_PREBUILT_DIR (see the comment on that branch). A CPU-only stub + # would ship, the loader would accept it, and capture would degrade to PipeWire + # without a word. + _assert_so_has_egl(so) + stage_libdrmtap_into_deb(so) + system2(f'rm -f {so}') + system2('rm -f tmpdeb/usr/share/rustdesk/libdrmtap.so tmpdeb/usr/share/rustdesk/libdrmtap.so.0') + else: + # Build it here, exactly as the flutter deb path does (build_libdrmtap_so asserts the + # EGL backend itself). The library is independent of the staged binary. + stage_libdrmtap_into_deb(build_libdrmtap_so()) system2('mkdir -p tmpdeb/DEBIAN') generate_control_file(version) - if ships_so: + # Keyed on the EXPLICIT request, not on what happened to be staged: by here a --drm build has + # its library in tmpdeb whichever of the two shapes it came from. + if want_drm: retarget_control_to_drm_variant() system2('cp -a ../res/DEBIAN/* tmpdeb/DEBIAN/') md5_file_folder("tmpdeb/") @@ -671,7 +690,7 @@ def build_deb_from_folder(version, binary_folder, want_drm=False): system2('/bin/rm -rf tmpdeb/') system2('/bin/rm -rf ../res/DEBIAN/control') os.rename('rustdesk.deb', '../rustdesk-%s.deb' % version) - if ships_so: + if want_drm: os.rename('../rustdesk-%s.deb' % version, f'../{DRM_PACKAGE_NAME}-{version}.deb') os.chdir("..") diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index d1398a357..4f9e5bf0b 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -117,13 +117,20 @@ pub fn list_devices() -> Option> { ) } -/// Returns true only if `path` canonicalizes to a node directly under /dev/dri/. -/// This is the realpath gate the libdrmtap helper applied but the in-process -/// (direct) path does not, so the service must apply it itself. -pub(super) fn device_under_dev_dri(path: &str) -> bool { - match std::fs::canonicalize(path) { - Ok(p) => p.parent().map_or(false, |d| d == std::path::Path::new("/dev/dri")), - Err(_) => false, +/// The CANONICAL path, when `path` canonicalizes to a node directly under /dev/dri/, else `None`. +/// This is the realpath gate the libdrmtap helper applied but the in-process (direct) path does +/// not, so the service must apply it itself. +/// +/// Returning the resolved path rather than a bool is the point: a gate that answers yes/no leaves +/// the caller opening the ORIGINAL string, so every symlink component gets resolved a second time, +/// by the library, after the check -- a check-then-use window in which a component could be +/// repointed outside /dev/dri, in the root service. Callers must open the value this returns. +pub(super) fn device_under_dev_dri(path: &str) -> Option { + let p = std::fs::canonicalize(path).ok()?; + if p.parent() == Some(std::path::Path::new("/dev/dri")) { + Some(p) + } else { + None } } @@ -149,13 +156,17 @@ impl DrmReader { let device_cstr = match device { None => None, Some(d) => { - if !device_under_dev_dri(d) { + // Open the CANONICAL path the gate resolved, never the caller's string: handing the + // original back would make libdrmtap re-walk the symlinks after the check. + let Some(canonical) = device_under_dev_dri(d) else { log::warn!("DRM device {d:?} is not under /dev/dri; refusing to open"); return None; - } - match CString::new(d) { - Ok(c) => Some(c), - Err(_) => return None, // interior NUL + }; + match canonical.to_str().and_then(|s| CString::new(s).ok()) { + Some(c) => Some(c), + // Non-UTF-8 or an interior NUL. /dev/dri node names are neither, so this is a + // path we do not need to serve. + None => return None, } } }; diff --git a/libs/scrap/src/common/drm_render.rs b/libs/scrap/src/common/drm_render.rs index e5f89a602..2c9664d55 100644 --- a/libs/scrap/src/common/drm_render.rs +++ b/libs/scrap/src/common/drm_render.rs @@ -62,17 +62,16 @@ impl RenderConverter { // opens whatever path it is handed is a needless widening. let node_cstr = match node.filter(|n| !n.is_empty()) { None => None, - Some(n) => { - if !super::drm_reader::device_under_dev_dri(n) { + // Open the CANONICAL path the gate resolved, not the string that arrived over IPC: + // opening the original would re-walk its symlinks after the check (see + // device_under_dev_dri). + Some(n) => match super::drm_reader::device_under_dev_dri(n) { + None => { log::warn!("drm: render node {n:?} is not under /dev/dri; auto-selecting"); None - } else { - match CString::new(n) { - Ok(c) => Some(c), - Err(_) => None, // interior NUL - } } - } + Some(canonical) => canonical.to_str().and_then(|s| CString::new(s).ok()), + }, }; // SAFETY: `open_render` is a resolved C entry point; `node_cstr` outlives the // call, and NULL requests auto-selection of a render node.