mirror of
https://github.com/rustdesk/rustdesk.git
synced 2026-09-08 13:31:03 +03:00
drm: the polish list, and a correction to my own ABI floor
The version floor I added two commits ago was one release too low. drmtap_open_render and drmtap_convert_dmabuf are 0.4.9, but drmtap_grab_desc is 0.4.10, so a genuine 0.4.9 library passed the version gate and was then refused by the symbol gate with a message that called it a stale or pre-release build, which it is not. The floor is 0.4.10 now, the release where the whole split API exists, and the test lists 0.4.9 among the rejected versions with the reason. ExportLedger is deleted. DRM_FD_ELISION was false, so should_send_fd returned true at its first branch and about sixty lines of eviction and epoch machinery were unreachable, untested, in a security sensitive file. Why it was disabled is worth keeping, so here it is: eliding the fd on an fb_id the converter has already imported looks free, but the kernel can recycle an fb_id onto a different buffer with identical geometry and modifier, and the exporter cannot see the dma-buf inode that would tell the difference, so the elision can serve a stale EGLImage. Sending it is cheap, the converter imports once per buffer and closes the surplus fd, and libdrmtap's own cache keys on fb_id AND inode and can only re-import when it is handed a real fd. That reasoning now lives here instead of in dead code. The rest: - num_planes is clamped on the consumer before it reaches the C descriptor. The producer normalizes it and must be root, so this is only defense in depth, but the wire is the one place the value arrives from another process. - warm_availability returns early on X11. Nothing there can consume a DRM stream, and probing makes the ROOT service open DRM readers, so an X11 host running a drm build was paying that at every startup for a path it can never take. - drm_cursor_id no longer clones the cursor. The cursor service polls it at frame cadence to compare eight bytes, and a 256x256 cursor is 256 KiB. - The premultiplied ARGB pass-through is now documented as matching the XFixes path, since that is why it is correct rather than an oversight. - cfg hygiene: input_service.rs uses all(target_os = "linux", feature = "drm") like every other site, and active_uid_cached is gated with the feature too, which also removes a dead-code warning from drm-off Linux builds. - Nits: DrmConn is pub(crate) like its constructors, new_drm_listener is no longer async with nothing to await, and the two anyhow! plus return Err pairs are bail! as the codebase writes them. - DRM_CAPTURE_SECURITY.md moves to docs/ with the other docs, and its "no privileged child process is ever spawned" claim is corrected: an empty helper_path is not a disable switch in the C, find_helper searches six fixed paths and would exec one if the direct export ever failed. It is unreachable here for two independent reasons, the root service holds CAP_SYS_ADMIN so the direct path succeeds and the package builds no helper at all, and the paths are root-writable only, so the accurate statement is that this package never installs one, not that it can never happen. - The comments that narrated the review rather than the code are rewritten to say what the code does. One of them had also drifted: the convert context is opened before we answer with DrmStart, not before the handshake. Both configs build with no new warnings, 100 tests pass.
This commit is contained in:
@@ -170,8 +170,9 @@ impl DrmReader {
|
||||
/// the internal buffer. Returns (width, height). The returned slice is valid
|
||||
/// until the next grab. A non-32bpp scanout, an oversized/degenerate
|
||||
/// geometry, or a stride < w*4 is rejected with a hard error so the caller
|
||||
/// falls back to PipeWire (see the codex format finding). Errno failures map
|
||||
/// to WouldBlock (retry) or a hard error (tear down) as in the old path.
|
||||
/// falls back to PipeWire rather than encoding whatever the bytes happen to
|
||||
/// mean. Errno failures map to WouldBlock (retry) or a hard error (tear
|
||||
/// down) as in the old path.
|
||||
pub fn grab(&mut self) -> io::Result<(&[u8], usize, usize)> {
|
||||
// SAFETY: self.ctx is a valid context; frame is zeroed before the call
|
||||
// and released on every path.
|
||||
|
||||
@@ -10,7 +10,8 @@
|
||||
// import-once EGLImage cache it holds are THREAD-LOCAL inside libdrmtap: the
|
||||
// context MUST be created, used (`convert`), and closed (`drop`) on the SAME
|
||||
// thread (the consumer's `recv_thread`). Dropping it off-thread would strand the
|
||||
// cached EGLImages — the exact leak class behind the 0.4.8 OOM regression. The raw
|
||||
// cached EGLImages, which leaks a GPU context per capture session until the process is out of
|
||||
// memory. The raw
|
||||
// ctx pointer makes `RenderConverter` !Send/!Sync, which enforces that at the type
|
||||
// level.
|
||||
|
||||
@@ -54,7 +55,7 @@ impl RenderConverter {
|
||||
/// an empty/invalid path falls back to libdrmtap auto-selection. It opens no KMS
|
||||
/// card, spawns no helper, and needs no elevated capability. Returns `None` when
|
||||
/// libdrmtap is unavailable or too old to carry the split convert symbols (the
|
||||
/// loader refuses a pre-0.4.9 `.so` outright), or when no render node could be
|
||||
/// loader refuses anything below 0.4.10 outright), or when no render node could be
|
||||
/// opened (a locked-down seat with no `/dev/dri/renderD*` access) — the caller
|
||||
/// then degrades to the service-side CPU convert / PipeWire path. MUST be called
|
||||
/// on the thread that will later `convert()` and drop it.
|
||||
|
||||
@@ -148,7 +148,7 @@ 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);
|
||||
// Split-capture entry points (libdrmtap >= 0.4.9). REQUIRED (see below).
|
||||
// Split-capture entry points (libdrmtap >= 0.4.10). REQUIRED (see below).
|
||||
// `grab_desc` runs on the privileged export side; `open_render`/`convert_dmabuf`
|
||||
// on the unprivileged converter side.
|
||||
type FnGrabDesc =
|
||||
@@ -176,7 +176,7 @@ pub struct DrmtapLib {
|
||||
pub frame_release: FnFrameRelease,
|
||||
pub get_cursor: FnGetCursor,
|
||||
pub cursor_release: FnCursorRelease,
|
||||
// Split-capture symbols (libdrmtap >= 0.4.9). Not optional: a library that
|
||||
// Split-capture symbols (libdrmtap >= 0.4.10). Not optional: a library that
|
||||
// cannot do the split is refused at load time (see `abi_accepted`), so these
|
||||
// are plain pointers and the type system carries the guarantee that no
|
||||
// caller can silently take an in-process-convert path instead.
|
||||
@@ -209,20 +209,20 @@ const DRMTAP_ABI_MAJOR: c_int = 0;
|
||||
// major alone bounds nothing: every release it has ever made reports major 0,
|
||||
// and comparing only that accepts a library from before the split existed.
|
||||
//
|
||||
// 0.4.9 is where `drmtap_grab_desc` / `drmtap_open_render` /
|
||||
// `drmtap_convert_dmabuf` landed, i.e. the oldest library that can serve the
|
||||
// architecture this code implements: the privileged process exports the scanout
|
||||
// dma-buf and NEVER converts, so it never loads libEGL/libGLESv2. An older .so
|
||||
// has none of those entry points, and the only way to capture with it is the
|
||||
// in-process convert, in the ROOT service. That is precisely the property the
|
||||
// split exists to remove, so treat such a library as unusable and fall back to
|
||||
// PipeWire/portal rather than quietly pulling the vendor GL stack into the
|
||||
// 0.4.10 is the oldest release with the WHOLE split API: `drmtap_open_render`
|
||||
// and `drmtap_convert_dmabuf` arrived in 0.4.9, `drmtap_grab_desc` in 0.4.10.
|
||||
// That is the oldest library that can serve the architecture this code
|
||||
// implements, where the privileged process exports the scanout dma-buf and NEVER
|
||||
// converts, so it never loads libEGL/libGLESv2. Below it the only way to capture
|
||||
// is the in-process convert, in the ROOT service, which is precisely the
|
||||
// property the split exists to remove: treat such a library as unusable and fall
|
||||
// back to PipeWire/portal rather than quietly pull the vendor GL stack into the
|
||||
// privileged process because a stale file happened to be on the load path.
|
||||
//
|
||||
// The mirrored `#[repr(C)]` layouts above are unchanged across 0.4.9..0.4.15
|
||||
// (verified field by field against include/drmtap.h at both ends), so the floor
|
||||
// costs no compatibility that was real.
|
||||
const DRMTAP_MIN_MINOR_PATCH: (c_int, c_int) = (4, 9);
|
||||
const DRMTAP_MIN_MINOR_PATCH: (c_int, c_int) = (4, 10);
|
||||
|
||||
/// 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
|
||||
@@ -420,8 +420,10 @@ mod tests {
|
||||
fn abi_gate_rejects_a_library_from_before_the_split() {
|
||||
// The releases that predate drmtap_grab_desc. Accepting any of these means the
|
||||
// privileged service has no export-only path and converts in-process, which is
|
||||
// the whole thing the split was built to prevent.
|
||||
for (minor, patch) in [(3, 3), (4, 0), (4, 8)] {
|
||||
// the whole thing the split was built to prevent. 0.4.9 is in the list on
|
||||
// purpose: it introduced the convert half of the split but not the export half,
|
||||
// so it cannot serve the privileged side either.
|
||||
for (minor, patch) in [(3, 3), (4, 0), (4, 8), (4, 9)] {
|
||||
assert!(
|
||||
!abi_accepted(DRMTAP_ABI_MAJOR, minor, patch),
|
||||
"v0.{minor}.{patch} predates the split-capture API and must be refused"
|
||||
|
||||
Reference in New Issue
Block a user