From a35ed165088f6958d3cbb5b026adb936f1396aea Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Wed, 29 Jul 2026 19:34:02 -0300 Subject: [PATCH] drm: gate the libdrmtap ABI on the minor, and skip the warm probe on X11 Two items from the review that I had recorded as done and were not. The ABI check had a floor and no ceiling, so 0.5.0 and 0.9.9 passed. Under 0.x semver the minor is the breaking axis, and libdrmtap freezes only drmtap_device and drmtap_dmabuf_desc: drmtap_frame_info, drmtap_display, drmtap_config and drmtap_cursor_info are not frozen. A 0.5.0 adding one field to drmtap_frame_info still reports major 0, so we would have loaded it and read every field at the wrong offset, in the root service. It now requires the verified minor; a 0.5.x needs a deliberate bump after comparing the layouts. The unit test asserted the opposite of this, in as many words ("0.5.0 must pass"), so it was holding the hazard in place. Replaced. warm_availability ran on X11 too, where every consumer of the verdict sits behind an !is_x11() check, so the root service opened DRM readers for a path the session can never use. --- libs/scrap/src/common/drmtap_dl.rs | 51 ++++++++++++++++++++++++------ src/server.rs | 6 +++- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index 13e0bd445..2b9d85da3 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -224,12 +224,26 @@ const DRMTAP_ABI_MAJOR: c_int = 0; // costs no compatibility that was real. const DRMTAP_MIN_MINOR_PATCH: (c_int, c_int) = (4, 10); +// The MINOR series this build's mirrored structs were verified against. Under 0.x semver the minor is +// the breaking axis, and libdrmtap's own header freezes only `drmtap_device` and +// `drmtap_dmabuf_desc`: `drmtap_frame_info`, `drmtap_display`, `drmtap_config` and +// `drmtap_cursor_info` are explicitly NOT frozen. So a 0.5.0 that adds one field to +// `drmtap_frame_info` would be layout-incompatible while still reporting major 0, and a floor alone +// would load it and read every field at the wrong offset -- inside the root service. +// +// Refusing an unknown-newer minor means a libdrmtap 0.5.x needs a deliberate bump here, after +// re-checking the layouts field by field. That is the point: the check should fail closed on a +// library nobody has compared against, not assume forward compatibility a 0.x project does not offer. +const DRMTAP_ABI_MINOR: c_int = 4; + /// Whether a library reporting `major.minor.patch` may be loaded. Pure, so the -/// version rule is unit-testable without an .so to dlopen: the major must match -/// exactly (struct layouts track it) and (minor, patch) must be at or above the -/// floor that provides the split-capture API. +/// version rule is unit-testable without an .so to dlopen: the major AND the minor must match +/// exactly (unfrozen struct layouts track the minor under 0.x semver), and the patch must be at or +/// above the floor that provides the split-capture API. fn abi_accepted(major: c_int, minor: c_int, patch: c_int) -> bool { - major == DRMTAP_ABI_MAJOR && (minor, patch) >= DRMTAP_MIN_MINOR_PATCH + major == DRMTAP_ABI_MAJOR + && minor == DRMTAP_ABI_MINOR + && (minor, patch) >= DRMTAP_MIN_MINOR_PATCH } impl DrmtapLib { @@ -432,15 +446,34 @@ mod tests { } #[test] - fn abi_gate_accepts_the_floor_and_every_release_above_it() { + fn abi_gate_accepts_the_floor_and_later_patches_of_the_same_minor() { let (min_minor, min_patch) = DRMTAP_MIN_MINOR_PATCH; assert!(abi_accepted(DRMTAP_ABI_MAJOR, min_minor, min_patch)); - // 0.4.15 is what the deb ships today; the later ones guard against a floor - // comparison that only ever looks at `patch` (0.5.0 must pass, 0.4.15 too). - for (minor, patch) in [(4, 15), (4, 200), (5, 0), (9, 9)] { + // 0.4.15 is what the deb ships today. A later PATCH of the verified minor is fine: + // patch releases do not change the layouts. + for (minor, patch) in [(4, 15), (4, 200)] { assert!( abi_accepted(DRMTAP_ABI_MAJOR, minor, patch), - "v0.{minor}.{patch} is at or above the floor and must be accepted" + "v0.{minor}.{patch} is a patch of the verified minor and must be accepted" + ); + } + } + + #[test] + fn abi_gate_rejects_an_unknown_newer_minor() { + // Under 0.x semver the MINOR is the breaking axis, and libdrmtap freezes only + // drmtap_device and drmtap_dmabuf_desc -- drmtap_frame_info, drmtap_display, + // drmtap_config and drmtap_cursor_info are not frozen. A 0.5.0 that adds one field + // to drmtap_frame_info still reports major 0, so a floor-only check would load it + // and read every field at the wrong offset, in the ROOT service. Fail closed on a + // minor nobody has compared the layouts against; bumping is a deliberate act. + // + // This test replaces one that asserted the opposite ("0.5.0 must pass"), which was + // holding the hazard in place. + for (minor, patch) in [(5, 0), (5, 99), (9, 9)] { + assert!( + !abi_accepted(DRMTAP_ABI_MAJOR, minor, patch), + "v0.{minor}.{patch} is an unverified minor and must be refused" ); } } diff --git a/src/server.rs b/src/server.rs index 465217abd..be62e74d0 100644 --- a/src/server.rs +++ b/src/server.rs @@ -603,8 +603,12 @@ pub async fn start_server(is_server: bool, no_server: bool) { }); // Warm the DRM availability cache before any client connects, so the first connection does // not race a cold `_drm` probe and ship an empty display list ("No displays" + retry). + // Skipped on X11: every consumer of the verdict is behind an `!is_x11()` check, so probing + // there makes the root service open DRM readers for a path this session can never use. #[cfg(all(target_os = "linux", feature = "drm"))] - std::thread::spawn(drm_capturer::warm_availability); + if !scrap::is_x11() { + std::thread::spawn(drm_capturer::warm_availability); + } input_service::fix_key_down_timeout_loop(); #[cfg(target_os = "linux")] if input_service::wayland_use_uinput() {