diff --git a/.github/workflows/flutter-build.yml b/.github/workflows/flutter-build.yml index 83cacd1cb..90380d60c 100644 --- a/.github/workflows/flutter-build.yml +++ b/.github/workflows/flutter-build.yml @@ -2041,6 +2041,23 @@ jobs: *) echo "::error::build.py returned no '$want' feature: $FEATURES"; exit 1 ;; esac done + # Run the drm-gated unit tests HERE, because nowhere else does. The ordinary linux CI + # runs the suite, but `drm` is opt-in, so every test behind that feature -- the cursor + # rotation and hotspot ones especially -- has no continuous coverage at all otherwise. + # Debug rather than --release: these are pure logic tests and the release build is + # already the slow part of this job. + # A cargo test FILTER that matches nothing exits 0, so a rename would turn this step + # into a green no-op. Assert it actually ran something. + for filter in drm_capturer_tests ipc::test; do + out=$(cargo test --locked --lib --features "$FEATURES" "$filter" 2>&1) || { + echo "$out"; echo "::error::drm tests failed for filter '$filter'"; exit 1; } + ran=$(echo "$out" | sed -nE 's/^running ([0-9]+) tests?$/\1/p' | paste -sd+ | bc) + echo "$filter ran ${ran:-0} tests" + if [ "${ran:-0}" -lt 1 ]; then + echo "::error::filter '$filter' selected no tests; it was renamed or removed" + exit 1 + fi + done cargo build --locked --lib --features "$FEATURES" --release rm -rf target/release/deps target/release/build rm -rf ~/.cargo diff --git a/build.py b/build.py index a9fd3ea15..a32e5fccc 100755 --- a/build.py +++ b/build.py @@ -523,6 +523,11 @@ def build_libdrmtap_so(): f'libdrmtap at {src} is {got_sha}, expected {LIBDRMTAP_SHA} ' f'(stale checkout from a different pin; removed, re-run to re-fetch)') build_dir = os.path.join(src, 'build-pkg') + # Configure a fresh dir, or RE-configure one an earlier build left behind. Without the second + # branch a build-pkg created before this option was added keeps its old configuration, meson + # skips setup, and the compile produces a helper-enabled .so. The assertion below then fails + # the build rather than shipping it, so this is a developer-build correctness fix and not a + # security one -- but it turns a confusing failure into no failure at all. if not os.path.exists(os.path.join(build_dir, 'build.ninja')): # -Dhelper=disabled: rustdesk never uses the privileged helper. The capture context is # opened only in the root service (every drmtap_open lives in src/ipc/drm.rs), which @@ -532,6 +537,8 @@ def build_libdrmtap_so(): # of its owner or mode -- inside the ROOT process. The option compiles that path out # entirely. Requested by the maintainer on rustdesk#16242. system2(f'meson setup "{build_dir}" "{src}" --buildtype=release -Dhelper=disabled') + else: + system2(f'meson configure "{build_dir}" -Dhelper=disabled') # Build only the shared library, not the bundled helper binary or the static archive. Since # libdrmtap 0.4.11 the project is `both_libraries` (a version-scripted .so + a static .a), so the # bare `drmtap` target is ambiguous ("drmtap:shared_library" vs "drmtap:static_library"); ask for diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index fccc7fe87..e1faf2734 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -6,6 +6,7 @@ use super::drmtap_dl::{ }; use hbb_common::log; use std::ffi::CString; +use std::os::raw::c_int; use std::io; use std::os::fd::{FromRawFd, OwnedFd}; @@ -48,6 +49,51 @@ pub struct CursorSnapshot { /// (2, 6), which is 11.2 px from the truth; the centre is 1.4 px away. Checked against all 35 /// shapes of the installed theme at 24 px: making the test symmetric moves exactly that one shape /// and leaves the other 34 byte-identical, arrows included. +/// Fold everything that makes a cursor a DIFFERENT cursor into its id. +/// +/// The producer dedupes by comparing this against the last one it sent, so anything left out is +/// something a change in which the consumer will never be told about. Geometry and the hotspot are +/// the obvious ones: identical pixels at a new size or hotspot are a new shape. +/// +/// `hot_measured` belongs here too, and that is less obvious. It is not metadata about the cursor, +/// it selects what the consumer DOES with it: false means rotate the bitmap and re-infer a +/// hotspot, true means map the supplied point. So a sample whose pixels and `(0, 0)` hotspot are +/// unchanged but whose provenance flipped is a different cursor as far as the client is concerned, +/// and without this it would be deduped away and never sent. +fn cursor_id(hash: u64, width: u32, height: u32, hotx: i32, hoty: i32, hot_measured: bool) -> u64 { + let mut id = hash; + for v in [ + width as u64, + height as u64, + hotx as u32 as u64, + hoty as u32 as u64, + hot_measured as u64, + ] { + id ^= v; + id = id.wrapping_mul(1099511628211); + } + id +} + +/// Whether `hot_x`/`hot_y` are the driver's answer rather than a guess. +/// +/// `answered` is what `drmtap_cursor_hotspot_valid` said: `Some(v)` when it answered, `None` when +/// the symbol is absent (a library older than 0.5.6) or it reported that nothing recorded an +/// answer for this sample. +/// +/// When it answered, that is the answer, full stop - including `Some(true)` on a `(0, 0)` hotspot, +/// which is the whole reason the entry point exists: a para-virtualized driver really can put the +/// hotspot at the top-left corner, and the coordinates alone cannot tell that apart from a plane +/// that exposes no HOTSPOT_X/Y at all. When it could not answer, fall back to the old test. That +/// test is wrong in both directions, but it is what this code did before, and it keeps a deployed +/// older `.so` behaving exactly as it used to instead of changing under it. +fn hot_measured_from(answered: Option, hot_x: i32, hot_y: i32) -> bool { + match answered { + Some(measured) => measured, + None => hot_x != 0 || hot_y != 0, + } +} + pub fn infer_hotspot(rgba: &[u8], w: usize, h: usize) -> (i32, i32) { let (mut minx, mut miny, mut maxx, mut maxy) = (w as i32, h as i32, -1i32, -1i32); for (i, px) in rgba.chunks_exact(4).take(w * h).enumerate() { @@ -423,7 +469,20 @@ impl DrmReader { hash ^= p as u64; hash = hash.wrapping_mul(1099511628211); } - let hot_measured = c.hot_x != 0 || c.hot_y != 0; + // Ask the library where the hotspot came from instead of guessing from the + // coordinates. `hot_x/hot_y == (0, 0)` means two opposite things - the plane + // exposes no HOTSPOT_X/Y (every bare-metal driver), or it exposes them and the + // driver's answer IS the top-left corner - and the old test below cannot separate + // them: it overrides a real (0, 0) measurement with a guess from the bitmap, and + // on a driver without the properties it would trust a hotspot nobody published. + // Asked BEFORE cursor_release, which is what owns this sample. + let answered = self.lib.cursor_hotspot_valid.and_then(|f| { + let mut valid: c_int = 0; + // 0 = answered; -ENOTSUP = nothing recorded an answer for this sample, which + // is what a cursor read through a pre-0.5.6 privileged helper produces. + (f(&c, &mut valid) == 0).then(|| valid != 0) + }); + let hot_measured = hot_measured_from(answered, c.hot_x, c.hot_y); let (hotx, hoty) = if hot_measured { (c.hot_x, c.hot_y) } else { @@ -432,11 +491,7 @@ impl DrmReader { // Fold geometry + hotspot into the id: identical pixels with a changed size or // hotspot must count as a new shape, otherwise drm_capture_worker suppresses the // update (it dedupes by id) and the client keeps rendering the stale cursor. - let mut id = hash; - for v in [cw as u32 as u64, ch as u32 as u64, hotx as u32 as u64, hoty as u32 as u64] { - id ^= v; - id = id.wrapping_mul(1099511628211); - } + let id = cursor_id(hash, cw as u32, ch as u32, hotx, hoty, hot_measured); Some(CursorSnapshot { id, width: cw as u32, @@ -496,3 +551,53 @@ impl Drop for DrmReader { } } } + +#[cfg(test)] +mod hotspot_provenance_tests { + use super::{cursor_id, hot_measured_from}; + + /// The dedup key has to move when the provenance does, or the consumer is never told that the + /// same picture now means something different. This is the transition that produces it: the + /// properties become readable, the driver's answer is the corner, and the bitmap is unchanged. + #[test] + fn provenance_changes_the_cursor_identity() { + let guessed = cursor_id(0xabc, 24, 24, 0, 0, false); + let measured = cursor_id(0xabc, 24, 24, 0, 0, true); + assert_ne!( + guessed, measured, + "same pixels, same (0, 0), different meaning: the producer must not dedupe this away" + ); + // And the pre-existing parts still count, so this did not trade one blind spot for another. + assert_ne!(cursor_id(0xabc, 24, 24, 0, 0, true), cursor_id(0xabc, 32, 24, 0, 0, true)); + assert_ne!(cursor_id(0xabc, 24, 24, 0, 0, true), cursor_id(0xabc, 24, 24, 1, 0, true)); + assert_ne!(cursor_id(0xabc, 24, 24, 0, 0, true), cursor_id(0xdef, 24, 24, 0, 0, true)); + } + + /// The case the whole entry point exists for, and the one the old heuristic got backwards: a + /// para-virtualized driver that really publishes the hotspot at the image's top-left corner. + /// `(0, 0)` measured is still measured, and re-inferring one from the bitmap would move a + /// cursor the driver had already placed. + #[test] + fn a_measured_zero_hotspot_is_measured() { + assert!(hot_measured_from(Some(true), 0, 0)); + } + + /// And the mirror: when the library says it was NOT measured, that is the answer even if the + /// coordinates happen to be non-zero. The old test would have trusted them. + #[test] + fn the_librarys_no_beats_the_coordinates() { + assert!(!hot_measured_from(Some(false), 12, 11)); + assert!(!hot_measured_from(Some(false), 0, 0)); + } + + /// No answer available: either the symbol is absent (a library older than 0.5.6) or nothing + /// recorded one for this sample (a cursor read through an older privileged helper). Neither + /// is "it was a guess", so the pre-existing behaviour is kept rather than resolved either way + /// -- an older deployed .so must keep behaving as it used to. + #[test] + fn without_an_answer_the_old_heuristic_stands() { + assert!(!hot_measured_from(None, 0, 0)); + assert!(hot_measured_from(None, 12, 11)); + assert!(hot_measured_from(None, 0, 5)); + } +} diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index 0312c75bb..ddaaa6071 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -127,6 +127,12 @@ type FnGrabMapped = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_frame_info type FnFrameRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_frame_info); type FnGetCursor = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_info) -> c_int; type FnCursorRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_info); +/// `drmtap_cursor_hotspot_valid`, added in libdrmtap 0.5.6. Answers, for the sample in hand, +/// whether `hot_x`/`hot_y` were read from the driver's HOTSPOT_X/Y plane properties: 0 with +/// `*valid` set, `-EINVAL` on a null argument, and `-ENOTSUP` when nothing recorded an answer, +/// which is what a cursor read through an older privileged helper produces. +type FnCursorHotspotValid = + unsafe extern "C" fn(*const drmtap_cursor_info, *mut c_int) -> c_int; // Split-capture entry points (libdrmtap >= 0.4.10), required: `grab_desc` runs on the privileged // export side, `open_render`/`convert_dmabuf` on the unprivileged converter side. type FnGrabDesc = @@ -148,6 +154,10 @@ pub struct DrmtapLib { pub frame_release: FnFrameRelease, pub get_cursor: FnGetCursor, pub cursor_release: FnCursorRelease, + /// Optional: it only exists from libdrmtap 0.5.6. Absent means the library cannot say where a + /// hotspot came from, NOT that it was a guess; `drm_reader` keeps the old heuristic for that + /// case and nothing else changes. + pub cursor_hotspot_valid: Option, pub grab_desc: FnGrabDesc, pub open_render: FnOpenRender, pub convert_dmabuf: FnConvertDmabuf, @@ -278,6 +288,8 @@ impl DrmtapLib { }; let render_node: Option = lib.get(b"drmtap_render_node").ok().map(|s| *s); + let cursor_hotspot_valid: Option = + lib.get(b"drmtap_cursor_hotspot_valid").ok().map(|s| *s); // Log the load only now that every required symbol resolved: this fn still returns None on a missing one. let loaded_from = real .as_ref() @@ -309,6 +321,18 @@ impl DrmtapLib { points at and remove any leftover libdrmtap.so.0* beside it. {effect}" ); } + // Same shape as the check above, and for the same reason: a library that REPORTS a + // version which exports the symbol and then does not have it is a stale or + // hand-substituted object, and saying so beats silently taking the legacy path. + if (minor, patch) >= (5, 6) && cursor_hotspot_valid.is_none() { + log::warn!( + "libdrmtap at {loaded_from} reports v{major}.{minor}.{patch} but is missing \ + drmtap_cursor_hotspot_valid: it is a stale or pre-release build. Check what \ + the soname symlink points at. Cursor hotspot provenance falls back to \ + guessing from the coordinates, which cannot tell an absent HOTSPOT_X/Y from \ + a driver that really puts the hotspot at (0, 0)." + ); + } Some(DrmtapLib { _lib: lib, open, @@ -319,6 +343,7 @@ impl DrmtapLib { frame_release, get_cursor, cursor_release, + cursor_hotspot_valid, grab_desc, open_render, convert_dmabuf, diff --git a/src/ipc.rs b/src/ipc.rs index 42600eb89..9a8c628d4 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -555,11 +555,29 @@ pub enum Data { height: u32, hotx: i32, hoty: i32, - #[serde(default)] + /// Whether the hotspot came from the driver, or was inferred from the bitmap. + /// + /// Absent means the producer predates the field, and the default must then be TRUE, not + /// `bool::default()`. These messages are JSON over a unix socket between two processes + /// that are upgraded separately: an old root service can be streaming to a freshly + /// started `--server`, and its `{"hotx": 12, "hoty": 11}` carries no provenance. Read as + /// `false`, the new consumer would throw that hotspot away and re-infer one from the + /// upright bitmap, moving the cursor on a rotated display for the length of the upgrade. + /// Defaulting to true means "no provenance available, so behave as the old protocol did" + /// -- use the point the producer sent. It is not a claim that the value was measured; a + /// new producer that wants re-inference says `hot_measured: false` explicitly, which + /// serializes, so new-to-new is unaffected. + #[serde(default = "legacy_hot_measured")] hot_measured: bool, }, } +/// The default for a `hot_measured` that is not on the wire at all; see the field's docs. +#[cfg(all(target_os = "linux", feature = "drm"))] +fn legacy_hot_measured() -> bool { + true +} + #[tokio::main(flavor = "current_thread")] pub async fn start(postfix: &str) -> ResultType<()> { let mut incoming = new_listener(postfix).await?; @@ -2306,4 +2324,43 @@ mod test { fn test_select_server_uid_fails_when_multiple_servers_are_ambiguous() { assert!(select_server_uid_for_user_main_ipc(&[501, 502], None, false).is_err()); } + + /// The upgrade window: an old root service is still streaming when a new `--server` starts, + /// and its DrmCursor carries no `hot_measured` at all. Read as `false` the consumer would + /// discard a hotspot the producer did measure and re-infer one from the bitmap, which moves + /// the cursor on a rotated display until the old process is replaced. `bool::default()` is + /// the wrong default here; the right one preserves what the old protocol meant. + #[cfg(all(target_os = "linux", feature = "drm"))] + #[test] + fn a_drm_cursor_without_provenance_keeps_the_hotspot_the_producer_sent() { + // `Data` is adjacently tagged (`tag = "t", content = "c"`), so this is the real shape on + // the socket, not a simplified one. + let legacy = + r#"{"t":"DrmCursor","c":{"id":7,"width":24,"height":24,"hotx":12,"hoty":11}}"#; + let msg: Data = serde_json::from_str(legacy).expect("legacy DrmCursor must deserialize"); + match msg { + Data::DrmCursor { + hotx, + hoty, + hot_measured, + .. + } => { + assert_eq!((hotx, hoty), (12, 11)); + assert!( + hot_measured, + "a message with no provenance field must be treated as the old protocol did, \ + i.e. use the hotspot as sent, not re-infer it" + ); + } + other => panic!("expected DrmCursor, got {other:?}"), + } + + // And a NEW producer that really wants re-inference says so, which serializes: the + // default must not swallow an explicit false. + let modern = r#"{"t":"DrmCursor","c":{"id":7,"width":24,"height":24,"hotx":0,"hoty":0,"hot_measured":false}}"#; + match serde_json::from_str::(modern).expect("modern DrmCursor must deserialize") { + Data::DrmCursor { hot_measured, .. } => assert!(!hot_measured), + other => panic!("expected DrmCursor, got {other:?}"), + } + } }