From 6b93f339bac953a7a5bfcf16799d90ba1fffe918 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Tue, 28 Jul 2026 10:46:56 -0300 Subject: [PATCH] drm: bound the body read, and stop the empty key from aliasing displays From the second review bot on our fork. Two of these are real and one of them is mine from earlier today. A raw body read had no deadline. Only the header was bounded, and drm_read_full loops on readable() until it has the exact length, so a producer that wrote a header and then stopped (crashed, stopped, wedged) pinned the consumer receive thread forever. That thread is also the one that observes the stop flag, so every capturer rebuild would have stranded another thread and its render context. The whole body is bounded now, and an overrun is a hard error because the header is already consumed and the frame cannot be resumed. get_capturer_info collapsed an unknown connector identity to the empty string and then read and wrote the health map under it, so two unidentifiable displays shared one entry and one could demote the other. That is exactly the aliasing frame() refuses to take part in; I fixed one side of it this morning and left the other. The key is an Option now and both blocks skip when it is None: a display with no identity simply carries no health. Also from the same pass, smaller: - build.py validates the shape of DRMTAP_SHA and DRMTAP_REPO before they reach a shell command. Both are env-overridable and get interpolated, and beyond the injection argument, an abbreviated sha would defeat the point of pinning while failing in a much less obvious place. - the workflow's push path list is now identical to the pull_request one. It was missing four paths, so a push to master touching only those would have skipped re-verification. - the checkouts set persist-credentials: false, so the token does not stay in .git/config for the rest of the job. - a concurrency group supersedes a stale PR run, but never cancels a master run, whose whole purpose is to record that a commit was verified. Not taken: reading VCPKG_COMMIT_ID and FLUTTER_VERSION from a shared .env. There is no .env at the repo root, and the stock ci.yml and flutter-build.yml hardcode those same two values, so this matches what is already there. 101 tests pass, both configs build. --- .github/workflows/drm-capture.yml | 16 ++++++++++++++++ build.py | 10 ++++++++++ src/ipc/drm.rs | 22 +++++++++++++++++++++- src/server/drm_capturer.rs | 13 +++++++++---- 4 files changed, 56 insertions(+), 5 deletions(-) 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,