TheLanc3/rustdesk
1
0
Fork 0
mirror of https://github.com/rustdesk/rustdesk.git synced 2026-09-06 08:01:03 +03:00
Code Issues Packages Projects Releases Wiki Activity

drm: stop a delivered frame from erasing the two verdicts it says nothing about

Browse Source 
A deep review pass over the whole branch, run because a maintainer once found
two bugs here that nineteen rounds of an automated reviewer had missed. Three
findings, two of them the same root cause, all confirmed by re-reading the code.

The first frame of a session dropped the display's whole health entry. That is
right for the zero-frame streak, which is exactly the verdict a delivered frame
refutes, and wrong for the other two:

- `last_build`/`rapid_builds` exist for a display that delivers a first frame
  and then fails downstream every cycle. Wiping the cadence on that frame meant
  the flap guard could never reach RAPID_REBUILD_MAX in the one case its own doc
  comment describes. It was a guard that could not fire.
- `prefer_cpu` records which GPU exports a monitor, a property of the host, and
  is documented as following the monitor for the process run. Erasing it on the
  first frame it made possible meant every rebuild re-paid a dead dma-buf
  session: fail, learn, take the CPU path, forget, fail again. It never demotes,
  because the CPU session clears the streak each time, so it repeats for the
  process lifetime. Worse, the bit is set on the recv thread and was deleted on
  the encoder thread, so a convert failure racing a queued frame could destroy
  it inside the very session that learned it.

So reset only the streak. Only a topology change, where the GPU mapping really
can have changed, may still clear the convert verdict.

Second, `get_primary_index` was a second, weaker copy of the connector-to-output
matcher: name-only, with neither the unique-resolution step nor the layout-order
fallback the augmentation grew. On a compositor whose names do not normalize to
the DRM names it answered 0 while the geometry augmentation had matched that
display to a different output, so the advertised primary and the advertised
geometry disagreed. It now asks the same assignment, which makes them agree by
construction.

Third, packaging asserted half of what the deb claims. `assert_staged_binary_is_drm`
looked for the libdrmtap dlopen path, which `--features drm` alone also carries,
so a bundle built without `drm-wake` could still be named and documented as the
variant that wakes an idle-disabled display; it now requires the wake marker too.
And nothing anywhere checked that the libdrmtap being shipped is one the runtime
would accept: `abi_accepted` is the only validation of the pinned version and it
runs at dlopen time on the user's machine, so the pin and the gate could drift
and every existing assertion would still pass -- EGL markers say nothing about
the version, the CI symbol contract never calls drmtap_version(), and the deb
regex matches any version. Staging now applies the gate parsed out of the Rust,
so a green build cannot produce a deb whose capture can never start.
This commit is contained in:
Mariano Abad
2026-07-31 11:48:08 -03:00
parent 6088a37445
commit 98f8ea3ad6
2 changed files with 112 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
build.py
View File

@@ -509,6 +509,52 @@ def _assert_so_has_egl(so_path):
DRM_PACKAGE_NAME = 'rustdesk-unattended-wayland'
def assert_so_satisfies_the_runtime_abi_gate(so_path):
"""The .so we are about to ship must be one the RUNTIME will actually accept.
`abi_accepted` in libs/scrap/src/common/drmtap_dl.rs is the only place the pinned library's
version is ever validated, and it runs at dlopen time on the USER's machine. Nothing in the
build or in CI compared the two, so the pin and the gate could drift apart and every existing
assertion would still pass: the EGL check does not look at the version, the CI symbol contract
does not call drmtap_version(), and the deb-contents regex matches any `libdrmtap.so.0.X.Y`.
A green pipeline could therefore produce a deb in which DRM capture can never start, and the
only symptom on the host is one log line before it falls back to the portal.
So parse the gate out of the Rust and apply it here, to the object being staged. This is the
same rule, not a copy of the numbers: if someone bumps the constants, this reads the new ones.
"""
m = re.search(r'libdrmtap\.so\.(\d+)\.(\d+)\.(\d+)', os.path.basename(so_path))
if not m:
# Not a versioned soname (a local dev build, say). The gate cannot be evaluated, and
# inventing a verdict would be worse than saying so.
print(f'[drm] cannot read a version out of {so_path}; skipping the ABI-gate cross-check')
return
so_ver = tuple(int(g) for g in m.groups())
gate_src = open('libs/scrap/src/common/drmtap_dl.rs').read()
def _const(name):
mm = re.search(rf'const {name}: c_int = (\d+);', gate_src)
return int(mm.group(1)) if mm else None
major, minor = _const('DRMTAP_ABI_MAJOR'), _const('DRMTAP_ABI_MINOR')
mm = re.search(r'const DRMTAP_MIN_MINOR_PATCH: \(c_int, c_int\) = \((\d+), (\d+)\);', gate_src)
floor = (int(mm.group(1)), int(mm.group(2))) if mm else None
if major is None or minor is None or floor is None:
raise Exception(
'could not parse the libdrmtap ABI gate out of drmtap_dl.rs (DRMTAP_ABI_MAJOR / '
'DRMTAP_ABI_MINOR / DRMTAP_MIN_MINOR_PATCH). The gate moved and this check did not; '
'fix the check rather than removing it, or the pin and the gate can drift silently.')
accepted = so_ver[0] == major and so_ver[1] == minor and (so_ver[1], so_ver[2]) >= floor
if not accepted:
raise Exception(
f'the libdrmtap being packaged is {so_ver[0]}.{so_ver[1]}.{so_ver[2]}, which the '
f'runtime loader would REFUSE: drmtap_dl.rs accepts exactly major {major}, minor '
f'{minor}, patch >= {floor[1]}. Shipping it produces a deb whose DRM capture can never '
'start. Move the build pin and the gate together, or fix whichever one is wrong.')
print(f'[drm] libdrmtap {so_ver[0]}.{so_ver[1]}.{so_ver[2]} satisfies the runtime ABI gate '
f'(major {major}, minor {minor}, patch >= {floor[1]})')
def stage_libdrmtap_into_deb(so_path):
# Put the built libdrmtap object plus its soname symlink into the staged deb. Only the soname
# symlink is needed: libdrmtap is resolved by ABSOLUTE path (/usr/lib/rustdesk/libdrmtap.so.0) at
@@ -516,6 +562,7 @@ def stage_libdrmtap_into_deb(so_path):
# system-wide /etc/ld.so.conf.d search path, which would let this private library shadow a system
# library for every binary on the host (Debian Policy 10.2 forbids that). No ld.so.conf.d drop-in
# and no ldconfig trigger are shipped, so the stock postinst is used unchanged.
assert_so_satisfies_the_runtime_abi_gate(so_path)
so_basename = os.path.basename(so_path)
system2('mkdir -p tmpdeb/usr/lib/rustdesk')
system2(f'cp {so_path} tmpdeb/usr/lib/rustdesk/')
@@ -615,9 +662,14 @@ def build_flutter_deb(version, features):
DRMTAP_DLOPEN_MARKER = b'/usr/lib/rustdesk/libdrmtap.so.0'
# Present only when `drm-wake` is compiled in: the runtime option constant is itself
# #[cfg(feature = "drm-wake")] (src/ipc/drm.rs). The dlopen marker above cannot stand in for it -
# `--features drm` alone produces a binary that carries the dlopen path and NO wake code, and that
# is exactly the deb this assertion is here to refuse.
DRMTAP_WAKE_MARKER = b'enable-drm-display-wake'
def _carries_drmtap_marker(path):
def _carries_drmtap_marker(path, marker=DRMTAP_DLOPEN_MARKER):
# Chunked, with an overlap of len(marker)-1 so the marker cannot be missed at a chunk boundary:
# librustdesk.so is ~45 MB and there is no reason to hold it all in memory, and the `with`
# closes deterministically instead of relying on refcounting.
@@ -627,9 +679,9 @@ def _carries_drmtap_marker(path):
chunk = f.read(1 << 20)
if not chunk:
return False
if DRMTAP_DLOPEN_MARKER in tail + chunk:
if marker in tail + chunk:
return True
tail = chunk[-(len(DRMTAP_DLOPEN_MARKER) - 1):]
tail = chunk[-(len(marker) - 1):]
def assert_staged_binary_is_drm():
@@ -650,6 +702,18 @@ def assert_staged_binary_is_drm():
f'{DRMTAP_DLOPEN_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')
# And the WAKE half. `--drm` enables `drm-wake` too (see get_features), and the deb is named and
# documented as the variant that can reach a machine whose screen has gone dark. The dlopen
# marker above does not distinguish them: `--features drm` alone carries it and has no wake code
# at all. Asserting only the first half is how a deb can be named for a feature it does not have.
if not any(_carries_drmtap_marker(p, DRMTAP_WAKE_MARKER) for p in binaries):
raise Exception(
f'--drm was requested but the staged binary has no {DRMTAP_WAKE_MARKER.decode()} '
f'marker in {binaries or "any staged binary"}, so it was built without `drm-wake`; '
'refusing to package it as the unattended-wayland variant, which is named and '
'documented as the build that can wake an idle-disabled display. If this fired under '
'--skip-cargo, the cargo line that produced the bundle is missing the feature: '
'--features ...,drm,drm-wake')
def build_deb_from_folder(version, binary_folder, want_drm=False):

59
src/server/drm_capturer.rs
View File

@@ -144,8 +144,11 @@ fn display_info_of(display: i32) -> Option<DrmDisplayInfo> {
/// The three verdicts live together because they are three answers to one question, "can this display
/// be captured over DRM right now", and they feed each other: the rebuild cadence and the zero-frame
/// streak both end in the same demotion, and the convert verdict is what keeps a multi-GPU display
/// off the dma-buf path so it never gets there. An entry is dropped entirely the moment the display
/// delivers a frame, which is the single reset for all of it.
/// off the dma-buf path so it never gets there. A delivered frame resets the streak verdicts
/// (`zero_frame_streak`, `demotes`) and NOTHING else: it is evidence that a grab succeeded once, not
/// evidence about the rebuild cadence -- which exists for displays that deliver a first frame and
/// then fail -- nor about which GPU exports the scanout. The entry itself is bounded by connector
/// count, so it costs nothing to keep.
#[derive(Clone, Copy)]
struct DisplayHealth {
/// Consecutive capture sessions that ended without ever producing a frame. A display whose
@@ -486,10 +489,30 @@ impl TraitCapturer for IpcDrmCapturer {
self.cur_fmt = fmt;
if !self.got_frame {
// First frame of this session: DRM capture works for this display, clear its
// failure streak.
// failure streak. Clear ONLY the streak, not the whole entry.
//
// Dropping the entry here is what a first frame used to do, and it silently
// disarmed the two verdicts that a first frame says nothing about:
// - `last_build`/`rapid_builds` exist precisely for a display that DELIVERS a
// first frame and then fails downstream every cycle. Wiping the cadence on
// that frame meant the flap guard could never reach RAPID_REBUILD_MAX in the
// one case its own doc comment describes -- a guard that cannot fire.
// - `prefer_cpu` is a property of the HOST (which GPU exports this monitor),
// documented as following the monitor for the process run. It is learned by
// a failed dma-buf session and consumed by the next one, so erasing it on
// the first frame it made possible meant every rebuild re-paid that failed
// session. Worse, the set happens on the recv thread and this delete on the
// encoder thread, so a convert failure racing a queued frame could destroy
// the bit inside the very session that learned it.
// Only `drm_clear_prefer_cpu` (on a topology change, where the mapping really
// can have changed) may clear the convert verdict.
self.got_frame = true;
if let Some(key) = &self.connector {
DRM_DISPLAY_HEALTH.lock().unwrap().remove(key);
if let Some(h) = DRM_DISPLAY_HEALTH.lock().unwrap().get_mut(key) {
h.zero_frame_streak = 0;
h.demotes = 0;
h.since = Instant::now();
}
}
}
} else {
@@ -1596,23 +1619,31 @@ pub(super) fn get_display_infos() -> Option<Vec<DisplayInfo>> {
}
/// Index (into the cached DRM display list) of the compositor's PRIMARY output. DRM connector order
/// is not the compositor's primary, so match the compositor's primary (from the same Wayland source
/// the geometry augmentation uses) to the DRM list by normalized connector name; fall back to 0 when
/// unknown. Without this the first DRM connector is always streamed, which is the wrong initial
/// display whenever the primary is not connector 0.
/// is not the compositor's primary, so find which DRM entry was assigned the compositor's primary
/// output; fall back to 0 when unknown. Without this the first DRM connector is always streamed,
/// which is the wrong initial display whenever the primary is not connector 0.
///
/// It asks `assign_wayland_outputs` rather than re-matching by name, and that is the point: whatever
/// that function decides IS the geometry advertised to the client for each index, so deriving the
/// primary from the same assignment makes the advertised primary and the advertised geometry agree
/// BY CONSTRUCTION. Matching by name here separately was the second, weaker copy of the same
/// question -- it had neither the unique-resolution step nor the layout-order fallback, so on any
/// compositor whose output names do not normalize to the DRM connector names (or reports none at
/// all, which the Wayland display code has its own "nameless compositor" path for) it silently
/// answered 0 while the geometry augmentation had matched that display to a different output.
pub(super) fn get_primary_index() -> usize {
let list = match &*DRM_STATE.lock().unwrap() {
ProbeState::Available(_, list) => list.clone(),
_ => return 0,
};
let wl = scrap::wayland::display::get_displays();
if let Some(pw) = wl.displays.get(wl.primary) {
let pn = normalize_connector(&pw.name);
if let Some(idx) = list.iter().position(|d| normalize_connector(&d.name) == pn) {
return idx;
}
if wl.displays.is_empty() {
return 0;
}
0
assign_wayland_outputs(&list, &wl.displays)
.iter()
.position(|assigned| *assigned == Some(wl.primary))
.unwrap_or(0)
}
/// The DRM enumeration reports every monitor at physical size and origin (0,0) — it deliberately