diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index db62bd48d..5e7ac112d 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -6,6 +6,12 @@ name: DRM capture (opt-in drm feature) permissions: contents: read +# Supersede a stale run when a PR is pushed again; never cancel a master run, whose whole job is to +# record that a given commit on master was verified. +concurrency: + group: drm-capture-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + # Everything CI-side about the opt-in `drm` backend lives here, so the stock CI and release workflows # stay byte-identical to a build with the feature off. Nothing in this file runs unless a drm-related # path changes (or someone dispatches it by hand), so a PR that does not touch the backend pays nothing. @@ -32,13 +38,19 @@ on: push: branches: - master + # Deliberately the SAME list as the pull_request trigger above: a shorter one here means a push + # that touches only the missing paths (a squash merge, a direct push) skips re-verification. paths: - "libs/scrap/src/common/drm_reader.rs" - "libs/scrap/src/common/drm_render.rs" - "libs/scrap/src/common/drmtap_dl.rs" + - "libs/scrap/src/common/mod.rs" + - "libs/scrap/Cargo.toml" - "src/ipc.rs" - "src/ipc/**" - "src/server/drm_capturer.rs" + - "src/server/wayland.rs" + - "src/server/display_service.rs" - "build.py" - ".github/workflows/drm-capture.yml" @@ -66,6 +78,7 @@ jobs: uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: submodules: recursive + persist-credentials: false - name: Install prerequisites shell: bash @@ -121,6 +134,8 @@ jobs: steps: - name: Checkout source code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: false - name: Install libdrmtap build deps shell: bash @@ -199,6 +214,7 @@ jobs: uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: submodules: recursive + persist-credentials: false - name: Restore bridge files uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 diff --git a/build.py b/build.py index dc564a7e4..7e695817e 100755 --- a/build.py +++ b/build.py @@ -8,6 +8,7 @@ import zipfile import urllib.request import shutil import hashlib +import re import subprocess import argparse import sys @@ -339,6 +340,15 @@ def ffi_bindgen_function_refactor(): # libs/scrap/Cargo.toml). This commit is libdrmtap v0.4.15. LIBDRMTAP_REPO = os.environ.get('DRMTAP_REPO', 'https://github.com/rustdesk-org/libdrmtap') LIBDRMTAP_SHA = os.environ.get('DRMTAP_SHA', 'cbc5e6af5b353b6bc351072a27a5351d82ba66e3') +# Both are interpolated into shell commands below, and both are env-overridable, so validate their +# SHAPE before they get there. This is not only about a hostile environment: a truncated or +# abbreviated sha would otherwise reach `git fetch` and fail with something far less obvious than +# saying so here, and an abbreviated one would defeat the point of pinning. +if not re.fullmatch(r'[0-9a-f]{40}', LIBDRMTAP_SHA): + raise Exception( + f'DRMTAP_SHA must be a full 40-character commit sha, got {LIBDRMTAP_SHA!r}') +if not re.fullmatch(r'(https://|git@)[A-Za-z0-9._~:/@-]+', LIBDRMTAP_REPO): + raise Exception(f'DRMTAP_REPO does not look like a git remote url: {LIBDRMTAP_REPO!r}') def _single_real_so(paths, where): diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index 7e409967a..0857b9379 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -1138,7 +1138,12 @@ pub(crate) struct DrmConn { /// hostile/oversized length prefix). Distinct from the raw-body cap because a body can be a whole /// CPU-fallback frame. const MAX_DRM_JSON_BYTES: usize = 8 * 1024 * 1024; -/// Cap on a raw body read by `next_raw` (CPU-fallback BGRA / cursor RGBA). Covers a 256 MiB 8K +/// Total budget for one raw body, from the header that announced it. Generous on purpose: the body is +/// a full frame on the CPU path (33 MB at 4K) but it crosses a unix socket, so it is milliseconds in +/// practice and this only has to bound a peer that stopped. +const DRM_BODY_TIMEOUT_MS: u64 = 5_000; + +/// Cap on a raw body read by `next_raw_into` (CPU-fallback BGRA / cursor RGBA). Covers a 256 MiB 8K /// scanout (`DrmReader` bounds a frame to that) with margin. const MAX_DRM_RAW_BYTES: usize = 512 * 1024 * 1024; /// Control-buffer capacity for one SCM_RIGHTS cmsg carrying a single fd. `CMSG_SPACE(sizeof(int))` is @@ -1486,7 +1491,22 @@ impl DrmConn { /// `ConnectionTmpl::next_raw`, except that the caller owns the buffer so it can be recycled /// across frames. A raw body never carries an fd; a stray fd (protocol desync) is collected by /// `drm_read_full` and dropped/closed. + /// + /// Bounded as a whole: the header that announced this body has already been consumed, so a body + /// that never finishes cannot be resumed, and an overrun is a hard error that ends the stream. + /// Without the bound a producer that writes a header and then stops (crashed, stopped, wedged) + /// pins the consumer receive thread forever on `readable()`. That thread is also the one that + /// observes `stop`, so every capturer rebuild would strand another thread and its render context. pub async fn next_raw_into(&mut self, out: &mut Vec) -> ResultType<()> { + match timeout(DRM_BODY_TIMEOUT_MS, self.next_raw_into_unbounded(out)).await { + Ok(res) => res, + Err(_) => bail!( + "drm: raw body did not arrive within {DRM_BODY_TIMEOUT_MS}ms of its header; closing" + ), + } + } + + async fn next_raw_into_unbounded(&mut self, out: &mut Vec) -> ResultType<()> { // next_raw is not called through recv_msg_timeout2, so its progress flag is unused; pass the // field for signature parity (recv_msg clears it before its own reads). let mut prefix = [0u8; 4]; diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index d3d0694ef..d12b27658 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -1414,7 +1414,12 @@ pub(super) fn get_capturer_info( // Identity of the display being asked for, resolved ONCE and before any of the per-display maps // are locked: connector_key_of takes DRM_STATE, and nesting that inside a map lock would be the // one lock order this file does not otherwise have. - let key = connector_key_of(display_idx as i32).unwrap_or_default(); + // `None` when the display list does not describe this index (not enumerated yet, or out of + // range). Kept as an Option rather than collapsed to "": an empty key is a REAL key in the map, + // so two unidentifiable displays would share one entry and one could demote the other. That is + // the aliasing frame() already refuses to take part in, and both blocks below skip on None for + // the same reason. A display with no identity simply carries no health. + let key = connector_key_of(display_idx as i32); // Refuse a display already demoted (repeated zero-frame sessions, or a detected flap below), so // the video service uses PipeWire for it instead of rebuilding onto DRM forever. Per-display, not // a global DRM disable. @@ -1424,7 +1429,7 @@ pub(super) fn get_capturer_info( // The demote count itself is KEPT, so a display that fails again waits twice as long; only a // delivered frame erases it (frame() drops the entry outright). let mut map = DRM_DISPLAY_HEALTH.lock().unwrap(); - if let Some(h) = map.get_mut(&key) { + if let Some(h) = key.as_ref().and_then(|k| map.get_mut(k)) { if h.zero_frame_streak >= DRM_GRAB_MAX_FAILURES { if h.demoted() { bail!( @@ -1447,10 +1452,10 @@ pub(super) fn get_capturer_info( // resets the count, so a healthy display (built once, streams long) never accumulates. The // initial build counts 0, so demotion fires on the RAPID_REBUILD_MAX-th rapid rebuild — i.e. // the (RAPID_REBUILD_MAX + 1)-th build inside the window. - { + if let Some(key) = key.clone() { let now = Instant::now(); let mut map = DRM_DISPLAY_HEALTH.lock().unwrap(); - let h = map.entry(key.clone()).or_insert_with(DisplayHealth::new); + let h = map.entry(key).or_insert_with(DisplayHealth::new); h.rapid_builds = match h.last_build { Some(last) if now.duration_since(last) < RAPID_REBUILD_WINDOW => h.rapid_builds + 1, _ => 0,