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.
libdrmtap treats egl/glesv2 as OPTIONAL: without their headers and
pkg-config files meson silently builds a CPU-only stub. The stub still
exports every symbol the loader gates on, so nothing downstream notices,
and the split capture depends entirely on the unprivileged side
EGL-detiling the scanout it receives. The result is a build where DRM
capture quietly degrades to PipeWire on every tiled-scanout host, which is
most of them. Our CI asserts this on the .so it builds; a developer or
packager running build.py got no such check.
Assert on the artifact rather than passing -Degl=enabled: that option only
exists in libdrmtap past the pinned 0.4.15, and checking what was actually
produced also catches a stale or substituted object, which a build flag
cannot. Same two markers CI looks for, and for the same reason an ELF-level
check does not work: EGL is reached by lazy dlopen so there is no
DT_NEEDED.
The uinput refresh worker released UINPUT_REFRESH_BUSY on its two normal
exits only. The body locks several process-wide mutexes and does a Wayland
roundtrip, so an unwind there left the flag set for the process lifetime,
and every later hotplug then skipped the spawn and never reapplied the
uinput ABS range: the stale-range, wrong-output symptom the refresh exists
to prevent. This file already had the answer for the probe flag, one screen
away, and the hazard is called out in wayland.rs. Fixing one site and not
the other is the same miss as the hotplug maps.
The slot is deliberately handed back and re-taken mid-loop, so the guard
tracks ownership rather than releasing unconditionally: a plain RAII drop
would clear a flag a replacement worker owns.
drm_reader bounded only the destination (w*4*h) while the row loop reads up
to (h-1)*stride + w*4, so a large stride read past the mapping and could
overflow usize in y*stride. drm_render::convert already bounds stride*h;
the privileged half must not be the weaker of the two.
Also give the drm CI jobs a timeout, so a hung meson or vcpkg step fails in
an hour instead of six.
build_libdrmtap_so() tests it for truthiness, so an empty value means no
prebuilt directory. The gate compared it against None instead, and would
have demanded the opt-in for an override that was never going to happen.
Three ways to build a different libdrmtap than the pinned one (DRMTAP_REPO,
DRMTAP_SHA, DRMTAP_PREBUILT_DIR) were each silent, and the last skips the
sha verification entirely. The claim this feature rests on is that the
privileged capture library is the reviewed object at the pinned sha, so any
build that is not that one now has to say so: the overrides still work and
still cover local work and cross-builds, but they need
DRMTAP_ALLOW_UNPINNED=1 alongside them and the build prints what it did.
--drm on Windows or macOS was accepted and then dropped by get_features(),
so it produced a stock build that looked like a DRM one. Reject it.
Also test the _drm body-read deadline, which nothing exercised: the header
and the body are separate reads, so the caller budget does not cover the
second one and a regression there would silently reopen the stall.
The display list carries the CRTC mode and a frame carries the scanout
framebuffer. Those are two different numbers whenever a CRTC scales a
smaller buffer up to its mode, so such a display fails the geometry guard
on the FIRST frame of every session, having delivered nothing.
That path marked the session as having produced frames, which is what the
zero-frame streak uses to decide a display cannot be served over DRM at
all. So the demotion to PipeWire never armed and the display rebuilt until
the rapid-rebuild guard caught it seconds later, under a message about a
mid-session change that never happened.
Count it instead, through the same bookkeeping the stream-died path uses
(now one helper, so the two cannot drift), and say which of the two cases
the error is. The unit test asserted the old behaviour on a capturer that
had never delivered a frame, so it is split into the mid-session case it
meant to cover and the first-frame case it was silently locking in.
The review named two untested things: the accept-time authorization decision,
covered by the previous commit, and the availability/demotion state machine. The
demotion half got tests with the backoff work; this is the other half, what a
completed background refresh decides.
It is extracted rather than tested in place on purpose. The effects touch
process-global state, DRM_STATE and the failure counter, which parallel tests
cannot share, so a test driving them would be intermittent by construction, which
is the kind of test nobody ends up trusting. The decision itself has no such
problem, so it is now a total function over the probe result and the consecutive
failure count, and the closure applies it.
Two tests: the decision table, including that a run short of the threshold keeps
a working verdict and the threshold gives it up; and the symptom the policy
exists for, a root service that dies while this server lives, where every probe
fails from then on and the verdict has to be given up in bounded time, to Unknown
rather than Unavailable, because what we learned is about the producer and not
about the hardware.
104 tests pass, both configs build.
The review called the accept-time authorization decision the single most
important invariant in this PR, and noted it has no test. Half of it did:
drm_peer_authorized_matrix covers the uid rule. The other half, the
/proc/<pid>/exe identity match that stops a DIFFERENT program running as the
right uid from being handed the screen, did not.
We said last round that testing it needs a second process with a different
executable, so it was integration rather than unit work. That was too
pessimistic: the negative case needs ANY foreign executable, not a second build
of rustdesk, and /bin/sleep is one. So the test covers all three outcomes: our
own pid matches, a live process running another binary is rejected, and a peer
whose pid cannot be resolved is rejected rather than admitted.
The test synchronizes on the child having exec'd before it looks. spawn returns
while the child is still a copy of us, and until exec completes /proc/<pid>/exe
points at OUR binary, so reading it too early sees a match and the assertion
passes for the wrong reason. It failed exactly that way under the parallel suite
and passed when run alone. A real peer has necessarily exec'd and connected
before it can be authorized, so the window exists only in the test.
102 tests pass, three consecutive full runs, both configs build.
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.
Two findings from the review bot on our own fork, both worth taking.
The caller waited HANDSHAKE_TIMEOUT_MS + 500 for the receive thread to hand back
the display list, but that thread is allowed to spend more than that: the connect
budget, and then recv_msg_timeout2 applies its argument twice in the worst case,
once waiting for the first byte and once for the body. So on a slow connect the
outer timer fired first and abandoned a handshake that was still inside its own
budget. The wait is now derived from those parts rather than written as a
constant, so changing either one cannot silently invert the relationship again,
and the two connect sites use the named constant instead of a literal.
The cursor cache insert shadowed hcursor under a cfg, so the same line meant the
requested id in one build and the served id in the other. It is a separate name
now, with the reason on it.
Not taken, and why: the bot also suggested making DrmCursorData carry width and
height as u32 to match the wire. They are i32 because that is what they feed,
protobuf CursorData declares both as int32 and platform/linux.rs assigns them
straight across. One cast has to exist somewhere, and it belongs at the boundary
where the values are already being validated, not at the consumer.
101 tests pass, both configs build.
CodeQL flagged the new workflow for not declaring permissions, which is fair:
every job here only checks out, builds and tests, and the artifact up/download
in the deb job authenticates with the runtime token rather than this one, so
contents: read is the whole requirement. Declared at the workflow level so the
reusable bridge workflow it calls inherits the same bound.
The stock workflows do not declare it either, but they are upstream's and this
feature does not touch them; a new file can start out right.
The three per-display verdicts are three answers to one question, can this
display be captured over DRM right now, and they already fed each other: the
rebuild cadence and the zero-frame streak 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. They are one struct now, keyed by connector identity.
This also closes a real leftover from H2. Two of the three maps were re-keyed by
identity then; the rapid-rebuild map was not, and stayed keyed by list index. A
hotplug that renumbers the list therefore moved a flap verdict onto whichever
monitor took that slot, which is the same defect in the third map. There is no
index-keyed per-display state left.
Behaviour is otherwise the same, with one improvement that falls out of the
merge: when a demotion cooldown expires, clearing the streak now keeps the
display's other state rather than replacing the whole entry, so a build cadence
and a convert verdict survive a retry the way they always should have.
One test for the demoted predicate, including that a higher demote count still
holds a display that a lower one would have released. 101 tests pass, both
configs build.
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.
The CPU fallback moved a scanout four times: the producer packed it, the kernel
carried it, next_raw allocated and zeroed a fresh buffer to read it into, and the
consumer copied that into the slot. At 4K30 the last two are about 8 GB/s of
memory traffic that does nothing.
next_raw_into reads the body straight into a buffer the caller owns, so the
kernel copy lands where the frame is going to live, and resize costs nothing once
a buffer has seen one frame of that size. The frame buffers then circulate
instead of being freed and reallocated: whatever a new frame displaces goes back
on offer, both when the encoder consumes one and when a frame is superseded
before anyone reads it. The dma-buf path still copies once, because the convert
output is borrowed from the render context and only lives until the next convert,
but it copies into a recycled buffer and does it outside the slot lock, so a
multi-megabyte memcpy no longer holds the encoder off the slot.
Steady state is now one allocation for the whole session on both paths, and the
CPU path carries the pixels twice instead of four times.
The cursor body reads into its own buffer and is moved into the cursor cache
rather than copied; it is small and rare, so it stays out of the frame recycler.
Two tests: the raw body round trip now also covers a shorter body reusing the
buffer, so a stale tail cannot survive into it, and a new test asserts the frame
buffers circulate by allocation identity rather than by inspection. 100 tests
pass, both configs build.
M5: the greeter fallback tested the wrong thing. start_server reports whether the
SPAWN succeeded, so a greeter account that cannot actually run the server, a
nologin shell or a hardened home, leaves a child that exits at once; the loop
sees only that the child is gone and respawns it as the greeter forever, never
reaching the root fallback, and the login screen becomes un-remotable on a host
where it used to work. It now requires the child to still be alive after a one
second grace before accepting it. A server that dies later than that is a
different, transient failure and the existing restart throttle already bounds it.
While there: the whole greeter branch is now inside the drm cfg, so the drm-off
build is upstream's single start_server line again rather than a run_as_greeter
variable that is always false.
M10: two monitors of the same model and resolution whose names do not normalize
to the compositor's matched no output at all, so both kept the DRM origin, which
is (0,0) for independent CRTCs. The client stacks them and injected coordinates
hit the wrong monitor with certainty. Unmatched connectors now take the next free
output in layout order, preferring one of the same physical size, and say so in
the log. That is at worst a swap of two identically sized rectangles, and the
layout stays coherent. The same pass also stops one output being claimed by two
connectors, which the unique-resolution rule allowed.
The assignment is now a pure function, so the cases are testable without a
compositor: five tests cover the naming difference, the identical-monitor case,
the double claim, name match beating the fallback, and more connectors than
outputs. 99 tests pass, both configs build.
The DRM path sets the uinput absolute range itself, because it bypasses
check_init. That copy awaited update_mouse_resolution raw, and it was missing
three things check_init has sixty lines above it.
No timeout: uinput set_resolution reads its reply with no timeout of its own, so
a hung uinput socket blocked every video-service start on this branch, and wedged
the hotplug worker inside rt.block_on with UINPUT_REFRESH_BUSY latched true,
after which every later hotplug refresh was silently skipped for the process
lifetime. It is bounded at 3 s now, the same bound check_init uses.
No bookkeeping: it never called set_wayland_uinput_rect or
set_wayland_layout_baseline, which is why the #15601 layout-drift remap never
activated on the DRM path. Both are recorded now, and only after a successful
apply, so a transient failure is retried rather than remembered as applied.
No cache invalidation: the cached Wayland layout can predate compositor changes
made while no session was active, which is the case #15601 is about. Dropped
first, as check_init does.
It also stops reprogramming the device when the range has not changed (M6): a
display in a rebuild loop called this about once a second, and reapplying an
identical range is an IPC roundtrip plus a uinput reconfiguration under a user who
may be at the console. The layout baseline is still re-snapshotted on every call,
since it is what the client coordinates are measured against.
Left as a separate copy rather than folded into check_init: check_init ships in
every Linux build and the standing rule for this feature is that the drm-off
build does not change by a line. Both configs build, 94 tests pass.
M4: a dead producer left the availability verdict positive forever. The
background refresh keeps a positive verdict on a failed probe, which is right for
one failure and wrong for a run of them: if the root --service dies while this
--server lives, every probe fails, the cached list keeps being advertised, and
every display restart-loops. Three consecutive failures now drop the verdict to
Unknown, not to Unavailable, because the evidence is about the producer and not
about the hardware, so the next enumeration probes from scratch. The cold probe
also resets its own failure budget on success: it was never reset, so the five
strike allowance was spent once per process and a later probe demoted on its
first failure.
M6: a display that can never be grabbed churned PeerInfo about every 35 seconds
for the life of the process, because the cooldown was flat: demote, wait 30 s,
get advertised online, burn four sessions in a few seconds, demote again. The
cooldown now doubles per demote cycle up to 8 minutes. Recovery is unchanged in
the way that matters, since the count is erased the moment the display delivers a
frame rather than decaying with time, so a monitor that comes back is served
immediately.
Also, while changing that map: a zero-frame session on a display with no
connector identity was recorded under the empty key, which is the same aliasing
H2 removed for indexes, one unidentifiable display would have demoted the next
one. It is skipped now, as the comment above it always claimed.
Two new tests cover the backoff schedule and the reported 35 second cycle. 94
tests pass, both configs build.
M1: authorization is now done on the blocking pool. It reads the active session
uid, which on a cache miss forks loginctl, and the socket is 0666 so any local
uid can make us do it. The same call exists for _service, but this runtime is
shared by every live capture stream, so a stall here hitches frames instead of
delaying one config sync.
M2: the handshake was a loop that ignored unexpected messages, which restarted
the ten second budget on each one, so a peer sending junk just inside the timeout
held a worker thread and one of the eight connection slots for as long as it
liked, and eight of them denied DRM capture entirely. It is one receive now, and
anything that is not DrmStart closes the connection: the consumer answers the
display list with DrmStart and nothing else, so there is nothing legitimate to
skip past.
M8: dropped the extra unauthorized-connection warn. log_rejected_service_connection
inside the authorization already logs the rejection with the peer and active uid
and rate limits it to one line per five seconds, which is exactly what a
world-connectable socket needs; the second line had no throttle and handed anyone
who can connect an unbounded log write.
Both configs build, 92 tests pass.
src/ipc.rs is the file every unrelated IPC change has to be read through, and
this branch had grown it from 2227 lines to 4112. Move the DRM half out, into
the same #[path] submodule form the file already uses for ipc/auth.rs and
ipc/fs.rs, so it lands as ipc/drm.rs beside them.
What moves: the two payload structs, the producer that runs in the root
--service, and the bespoke SCM_RIGHTS framing the channel needs because
Framed/BytesCodec cannot carry ancillary data, plus their tests. What stays is
the Data variants, which belong to a shared enum and cannot live anywhere else,
and three re-exports so every existing call site keeps the path it already uses.
ipc.rs is 2285 lines now, 58 above upstream instead of 1885. The move is
content-identical: the only edits are the 39 per-item cfg attributes, redundant
now that the module is gated once at its declaration, and the test module cfg
that becomes a plain cfg(test). Checked by extracting the moved ranges from the
previous commit and comparing them line by line against the new file. Both
configs build with no new warnings and the same 92 tests pass, 14 of them the
drm ones that moved.
The root --service must never load libEGL/libGLESv2: the point of the split is
that it exports the scanout dma-buf and the unprivileged --server converts. Two
paths could still break that, both because the loader accepted a library too
old to export.
drm_prewarm() called grab() when the loaded .so had no drmtap_grab_desc, and
grab() maps and detiles, so the privileged process pulled in the vendor GL stack
at startup, before any consumer had asked for a frame. The per-connection
capture loop then did the same for every frame, through the CPU fallback.
The version guard could not prevent it: it compared the ABI major only, and this
library is still 0.x, so every release it has ever made passed. Add a floor at
0.4.9, where the split entry points landed, and require the three split symbols,
which also rejects a build that reports a new enough version without carrying
them. That is not hypothetical: a pre-release stamped 0.4.15 shipped without the
multi-GPU accessors. Both refusals fall back to PipeWire/portal and say which
file and which symbols, at warn level.
The split symbols are no longer Options, so the type system carries the
guarantee instead of a convention. What is left of the CPU path is only what it
was meant to be: the consumer has no render node of its own, or the seat exports
no transferable dma-buf. Both are facts about the hardware, with no alternative
that keeps the stream, and neither is a property of which file was on the load
path.
Verified against the real library on i915. With 0.4.15 the export path captures
a tiled XR30 scanout and libEGL stays out of /proc/self/maps, while the old
grab() branch maps it, so the finding reproduces. A stub reporting 0.4.8 and a
stub reporting 0.4.15 without the split symbols are both refused, each with its
own diagnostic. The mirrored repr(C) layouts are unchanged across 0.4.9 to
0.4.15, checked field by field against include/drmtap.h at both ends, so the
floor costs no compatibility that was real.
A resolution DECREASE wedged the stream. The encoder is sized once, from
CapturerInfo at capturer build time; check_display_changed returns None on Wayland,
so the periodic display-changed broadcast never fires there; and convert_to_yuv only
bails when the source is LARGER than the destination. A smaller frame therefore
passed all three and was encoded into the previous canvas, leaving stale content
along the right and bottom edges for the rest of the connection. An increase
recovered only by accident, because convert then refused and the service rebuilt.
This is ours to contain rather than merely inherited: the DrmDisplaysChanged
handler re-broadcasts the new geometry through SYNC_DISPLAYS, so the client layout
and the pixels it receives actively disagree, where before there was no topology
signal at all.
The capturer now records the geometry its session was built with and returns a hard
error from frame() when a dequeued frame differs, which routes a shrink through the
same rebuild an enlargement already takes. got_frame is set first so a session that
did deliver frames is not counted as one of the zero-frame sessions that demote a
display to PipeWire.
The general fix belongs to the Wayland path rather than to this backend, and is
filed separately as #15695.
Four tests cover it, the first in this file: the matching size is delivered, a
smaller and a larger frame both end the session, and an unknown session size stays
out of the way instead of rejecting everything.
The service binds a stream to (device, crtc_id), which survives a topology change.
Everything on the consumer side addressed it by list index, which does not:
drm_enumerate_all_displays concatenates per-card lists, so plugging or unplugging a
monitor renumbers every display after it. Two consequences, one live and one
remembered.
Live: a running stream kept sending monitor A while the advertised list, and so the
client layout and the injected-input rect, had come to mean monitor B. It only
resolved if the stream happened to fail on its own. The stream now records what it
was bound to and ends itself when its index stops meaning that, which routes the
change through the rebuild the video service already does.
Remembered: the zero-frame failure counts and the prefer-cpu verdicts were keyed by
index too, so after a renumbering one monitor could inherit another's demotion or be
forced onto the CPU convert path for a mismatch that was never its own. Both are now
keyed by device plus connector name. The reasoning was already written down for one
of these, in the comment above the prefer-cpu clear, and applied only there.
That bulk clear is gone with it. It existed to limit the damage of index aliasing;
with identity keys it would instead throw away a correct verdict, which costs a real
convert failure to relearn, on every unrelated hotplug.
Also fixes the drm workflow to skip the two tests the stock CI already skips. Both
need a display server and fail on any headless runner, so the job would have gone
red for a reason that has nothing to do with this feature. Verified by running the
exact command: 88 tests, including the size_of::<Data>() assertion that the old
name filter was hiding.
generate_control_file goes back to upstream byte for byte: no extra parameters, no
conditional inside it. The variant instead rewrites the control file that function
just produced, so everything specific to the consent-free package lives in added
code rather than in the shared one. That rewrite fails loudly if either anchor line
stops matching, so a future upstream change to the control layout cannot quietly
yield a variant deb wearing the stock package name.
finalize_deb is gone. It had pulled the tail of both deb builders into one shared
helper, which is a refactor of a path the feature has no business touching. Both
builders now carry their upstream tail verbatim, with the drm work added as three
guarded blocks: stage the library, retarget the control, rename the output. With
the feature off, every line is upstream's.
Verified rather than argued, by building both packages with this script:
the drm deb is Package: rustdesk-unattended-wayland, carries Conflicts, Replaces
and Provides on rustdesk, has libdrm2, libegl1 and libgles2 appended to Depends,
and ships libdrmtap.so.0.4.15 plus its soname symlink. The stock deb is
Package: rustdesk, carries none of those three fields, and contains no libdrmtap
file at all.
flutter-build.yml goes back to upstream byte for byte. Three separate changes to
the stock release path disappear with it: the drm variant built inside the release
container, the snapshot and restore of the stock flutter bundle that existed only
to keep the drm relink out of the archlinux package, and the narrowing of the
publish glob to keep the consent-free deb off the public release.
The deb now builds in the drm workflow instead, which also removes the failure
mode the old placement forced: the whole block had to run in a subshell ending in
`|| echo WARN` so a drm-only breakage could not abort the stock publish steps,
which meant every failure in it, from the fetch to meson to packaging, kept the
job green and silently stopped producing the deb. A separate job can just fail.
The bridge generator is a reusable workflow, so this calls the stock one rather
than duplicating the codegen.
The deb is asserted rather than trusted: build.py can exit 0 without producing a
package, so the job checks the file exists and that it carries both the real
libdrmtap object and its soname symlink. It stays an artifact and never a release
deliverable, and it is built on the runner rather than in the old container the
stock debs use, so its glibc floor is higher than a released package.
The instruction was that nothing outside the feature should change while the
feature is off, and the runtime code honors that, but the build plumbing did not.
Start undoing that.
ci.yml goes back to upstream byte for byte. The drm test step it carried now lives
in a new workflow that only fires when a drm path changes, so a PR that does not
touch this backend pays nothing for it.
That new workflow also runs the whole rustdesk-crate test set with the feature on
rather than filtering by the `_drm` test names, because the name filter skipped
the sibling assertion that bounds `size_of::<Data>()`, which the new DmabufDesc
variant grows.
It gains a second job that fetches libdrmtap at the pinned commit, builds the .so
and then asserts the contract the runtime depends on: every symbol the loader
resolves, derived from the loader source so the two cannot drift, plus evidence
that the EGL detile path is really compiled in. libdrmtap degrades to a CPU-only
stub when the egl/glesv2 pkg-config files are absent on a build host, and nothing
downstream noticed. Note the check looks for the dlopen target name and the import
call, not for DT_NEEDED: EGL is loaded lazily on purpose so the privileged process
never links the vendor GL stack, so an ELF-level check reports a false negative on
a correct library.
libs/scrap/Cargo.toml keeps only the added feature: the unrelated blank line before
[dependencies.hwcodec] is restored, and the comment no longer describes DRMTAP_REF,
which no longer exists. The feature is now drm = ["wayland"] because all three drm
modules live inside the wayland arm of common/mod.rs, so scrap/drm alone compiled
nothing; it worked only because the root crate always enables scrap/wayland.
`git clone --depth 1 --branch main` fetches only the tip of that branch, so the
moment upstream pushes to libdrmtap `main` the pinned commit is no longer present
in the shallow clone at all: the build fails on an unreachable object rather than
on a mismatched pin, and it fails for a reason that has nothing to do with the
checkout being wrong. In the release workflow the whole block is wrapped so the
job stays green, which means the drm deb would simply stop being produced without
anyone noticing.
Fetch the sha directly instead. No branch or tag name takes part in the build now,
so it survives every upstream push and cannot be affected by a ref being moved or
repointed. DRMTAP_REF is gone, along with the regex that validated it.
The post-fetch sha check stays, with a narrower job: a fetch by sha cannot resolve
to anything else, so it now guards a reused checkout left at a different pin, which
is exactly what a version bump leaves behind. It still removes that tree so the
next run re-fetches cleanly.
build.py is now the single source of truth for the pin.
The security document described the CPU fallback without saying when it is
taken, and the multi-GPU safety fallback added in this branch was not mentioned
at all. Enumerate the four cases, including the one where the service could not
name the exporting GPU on a host with several render nodes, and note that a
single-render-node host keeps the DMA-BUF path.
Four corrections from a review pass over the previous two commits.
Count only a render node whose name is renderD followed by a numeric minor.
The prefix test also matched something like renderD.backup, which would have
inflated the count and pushed a genuinely single-GPU host onto the CPU path.
Log the load only after every required symbol resolved. load() still returns
None when one is missing, so announcing success first could print "libdrmtap
loaded" and then "libdrmtap not available" for the same library.
Name only the capability each absent symbol costs: a library missing just
drmtap_render_node loses exporting-GPU selection, one missing just
drmtap_list_devices loses multi-GPU enumeration, and the previous wording
claimed both were gone in either case.
Fix the security document's audit step. The dlopen names the symlink by
absolute path and the package registers no linker directory, so a leftover
object beside it is not loaded on its own; what matters is where the symlink
points, and a leftover only matters as what a stray ldconfig would repoint it
to. Ask the auditor to read the symlink target instead.
Two hours went into a corrupted capture whose only symptom was a clean log
saying "libdrmtap loaded: /usr/lib/rustdesk/libdrmtap.so.0 (v0.4.15)". The
library behind that soname symlink was a pre-release 0.4.15 that reported the
version but did not export drmtap_render_node, so the service silently stopped
naming the exporting GPU. The log named the symlink it asked for, which is not
evidence of anything, and the version it printed came from the library itself,
which was the part that lied.
Log the file the absolute candidate actually resolves to, and warn when a
library reports 0.4.15 or newer while missing drmtap_render_node or
drmtap_list_devices, naming that file: a version that claims features the
symbols do not back means a stale or pre-release build, and the effect is
invisible otherwise. Only the absolute candidate is resolved, because dlopen
does not search the process CWD for a bare soname while canonicalize would.
Also correct two places that no longer matched the code: the security document
still described an /etc/ld.so.conf.d drop-in and an ldconfig trigger that
build.py deliberately does not ship (the .so is dlopened by absolute path and
the package makes the soname symlink itself), and the comment above the render
node lookup still said an unnamed exporter always falls back to auto-selection.
The converter binds the render node the service names for a display, and falls
back to auto-selection when that name is empty. An empty name is what an older
libdrmtap produces: the service resolves it with drmtap_render_node, which only
exists since 0.4.15, and rustdesk dlopens libdrmtap.so.0 by soname, so the
runtime library can be older than the one the build was pinned to.
Auto-selecting is not safe there. On a single-SoC multi-device host the wrong
choice does not fail: a Jetson Orin exports the scanout from nvidia-drm while
the first render node belongs to tegra, and importing the scanout on the tegra
node SUCCEEDS and yields corrupted pixels. There is no convert error, so the
prefer-cpu bit never learns anything and the stream simply looks broken with a
clean log.
Request the CPU-converted path instead whenever the exporter is unnamed and the
host exposes more than one render node: the service converts on the device it
already has open, which is correct by construction. Hosts with a single render
node have nothing to pick wrong and keep the dma-buf path untouched.
Verified on a Jetson Orin Nano, the two-device host: with a libdrmtap that
lacks drmtap_render_node the capture used to come through visibly corrupted,
and now falls back to the cpu path and renders correctly. With 0.4.15 the
service names renderD129 and the dma-buf path is used as before.
A drmtap context is bound to a single DRM device, so the service enumerated one
auto-detected card and advertised only its monitors. On a multi-GPU host every
display driven by another card was invisible to the client, and its card-local
CRTC id could not have been opened through the wrong device anyway.
The service now enumerates every card (drmtap_list_devices, libdrmtap 0.4.15),
opens one reader per device, and merges their displays into the one list, each
tagged with its own card node and render node. DrmStart resolves the chosen
index to that display's device + CRTC and the worker reopens the right card;
the converter already binds the display's render node. Both new fields are
serde(default) and empty means the single auto-detected device, so a pre-0.4.15
.so and a mismatched-build peer keep the previous behaviour exactly.
Enumeration replaces the single-reader open in the pre-warm, the udev hotplug
refresh, and the per-connection handshake, so a hotplug on any card is picked up
and an all-monitors-off state now correctly publishes an empty list. The
per-connection cache refresh re-enumerates all cards rather than only the
connection's device, so serving one display never drops the others from the
next handshake.
Verified on a Jetson Orin (its two DRM devices, only card2 driving a display):
list_devices reports card2/renderD129 with one display, enumeration produces
exactly that display tagged to card2, and card1 (no active CRTC) is skipped -
no phantom, no regression on the single-display case.
The unprivileged converter opened its render context with
drmtap_open_render(NULL), letting libdrmtap auto-select. On a multi-GPU host
that can land on a different GPU than the one driving the display, and importing
a scanout across vendors can fail permanently on an incompatible tiling
modifier.
The service already knows the exporting device, so it now names its render node
(drmtap_render_node, libdrmtap 0.4.15) in each DrmDisplayInfo, and the consumer
opens the converter on that node. The field is serde(default) and empty means
auto-select, so a service and a server from mismatched builds still interoperate
and a pre-0.4.15 .so degrades to exactly the previous behaviour. The path is
realpath-gated to /dev/dri before it is opened, the same gate the capture device
gets, since it arrives over IPC. When the named node cannot be opened the
converter returns None and the existing need_cpu fallback runs the convert on the
exporting GPU service-side, which is the most correct place for it anyway.
Added a wire-compat test that a pre-render_node DrmDisplayInfo payload still
decodes (empty node) and a current one round-trips the node.
Two defects in the previous commit's staleness check.
The single-flight guard was still created inside the spawned closure, but that
commit added a DRM_STATE lock before the spawn. A poisoned lock there would
unwind past the flag with nothing to clear it, leaving DRM_PROBE_IN_FLIGHT set
and freezing every future probe. The guard is now taken immediately after the
flag is acquired and moved into the closure, so it covers the lock, the probe,
and a failed spawn alike. The explicit release on spawn failure is gone with it:
it was not merely redundant but wrong, since by then another refresh may have
acquired the flag and clearing it would let two probes run at once.
The staleness check itself compared Instant stamps, which made correctness
depend on an implicit invariant -- that every publish restamps -- spread across
ten call sites; a future publish that reused a stamp would defeat it silently.
DRM_STATE now carries an explicit generation, bumped by publish_probe_state,
which every write to the state goes through. Instants are left to serve only the
TTL checks. The failed-probe branch deliberately restamps without bumping: it
touches the TTL, not the verdict, so a concurrent probe loses nothing by
publishing over it.
query_displays() in the background refresh runs unlocked because it is slow, so
a hotplug push can publish a newer verdict while it is in flight; the refresh
then overwrote it with its own older result. Harmless while it only replaced the
list, but the previous commit made an empty result drop to Unavailable, so a
probe that started while the monitors were gone could disable DRM on a host
whose monitor had since come back.
The refresh now samples the stamp of the verdict it is refreshing and publishes
only if that stamp is still current. Every publish stamps a fresh Instant, so an
unchanged stamp means nothing republished in between -- equivalent to threading a
revision counter through every publish site, without having to keep all of them
in sync.
The review asked for two things when the last CRTC disappears: push the empty
topology to consumers, and stop advertising the removed displays. Only the first
was done. The positive-TTL refresh still discarded an empty probe result and kept
the previous list, so on an idle host -- where there is no live stream to carry
the hotplug push -- enumeration kept reporting displays that were gone, exactly
as described. It now transitions to Unavailable on an empty result, matching the
hotplug path, while a failed probe (transient open/EACCES, not evidence the
displays are gone) keeps the verdict and only restamps it.
The bound added in the previous commit was a timeout on the wait itself, so any
wake renewed it -- and cursor messages keep arriving while frames are gated, so
a consumer that had stopped acking but still moved its pointer would renew the
deadline forever and never be torn down. Track when we last held credit instead
and enforce the deadline against that, keeping the wait capped only so we still
wake to re-evaluate it when nothing arrives at all.
Follow-up to the previous commit, from an adversarial review that modelled the
loop with a real runtime, socket pair and worker thread.
Gating the worker only while a frame was already held was wrong: those grabs are
not wasted work, they keep the held frame fresh, because the coalescing below
lets each newer frame supersede it. Pinning the worker at that moment therefore
froze whatever frame happened to be in hand when credit ran out and shipped it
stale once the ack landed -- measured at ~91ms average staleness against ~2ms
with no gate at all. Gating on lack of credit alone, and waiting on the socket
whenever credit is out rather than only while holding a frame, keeps the CPU
saving (the worker still stops grabbing) with no staleness: the ack resumes the
worker and what goes out is a fresh grab. Modelled at 0ms staleness and the same
delivered-frame count, with 31 grabs versus 588 ungated. It is deadlock-free
because the socket is watched in exactly the states where the gate is set.
The no-credit wait is now bounded (5s). While gated the worker does not grab, so
it cannot advance its own MAX_STALLED watchdog; a consumer that stopped acking
without closing the socket could otherwise hold this connection, its worker
thread and the privileged DRM context open indefinitely.
Follow-up to the previous commit, from an adversarial review of it.
- The ack wake-up skipped the coalescing drain. When the socket arm of the
select won, there was no message to seed the drain loop with, so the channel
was never polled that iteration: a held frame could be sent while a strictly
newer one already sat queued, and a queued cursor waited for the next producer
message. Seed the loop from the channel when we woke on an ack instead.
- The loop could wait while holding a frame it was allowed to send. Credit
replenished by the top-of-loop drain was not consulted before entering the
select, so the frame waited for the worker's next message; if capture then
returned WouldBlock it sat there until the stall teardown. Take whatever is
queued without blocking in that case and fall through to the send.
- The capture worker no longer had any backpressure. Draining the channel every
iteration (needed so cursors keep flowing) means a full channel no longer
parks it, so a consumer converting at a fraction of the capture rate made the
privileged service keep grabbing frames that were then discarded -- a packed
copy per frame on the CPU path, a PRIME export on the dma-buf path. The worker
now skips the grab while the task is holding an undeliverable frame, and keeps
polling the cursor so the remote pointer stays live. The gate is deliberately
conditioned on holding a frame, not merely on having no credit: with nothing
held the task blocks in recv() and cannot observe an ack, so gating there
would stop the worker feeding it at all.
The comment claiming the bounded channel backpressures the worker is corrected.
The credit check sat at the top of the producer loop and continued on exhaustion,
so while a slow convert withheld its ack the loop never reached the code that
forwards cursor updates and pushes a changed display list: the remote cursor
froze and a hotplug went unreported until credit returned. The comment claimed
those were not credit-gated; structurally they were.
The loop now always receives and processes producer messages. Only the frame send
is gated: when credit is exhausted the newest frame is held back (latest-wins,
matching the existing coalescing) and flushed as soon as an ack lands, while
cursors and the topology push go out unimpeded. While a frame is held the loop
also waits on the socket, so an ack wakes it promptly rather than only when the
next frame arrives; both select arms are cancel-safe.
The worker released its coalescing slot and returned silently when the runtime
failed to build, leaving the uinput range stale for the new layout with nothing
in the log to explain it.
- Start the login-screen --server as the active seat0 greeter account instead
of root, so the DRM capture GPU/EGL convert never loads the vendor GPU
userspace in a privileged process. A genuine root graphical session has no
lower uid to drop to and stays root, and if the greeter spawn fails we fall
back to a root --server so the login screen stays remotable. Gated on the drm
feature so the non-drm build is unchanged.
- Bound the number of frames in flight on the `_drm` channel: the consumer acks
each frame it finishes converting and the producer only sends while it holds
credit, waiting on the socket otherwise. Without this the producer kept
writing descriptors into the socket faster than a slow convert drained them
and the consumer worked through an ever-growing backlog of stale frames. A
zero-byte read or write on the ack path is treated as a closed peer rather
than as success.
- Forward a display list that became empty (last monitor unplugged) instead of
dropping it, so the availability cache leaves Available rather than keep
advertising removed displays.
- On a topology change, invalidate the Wayland geometry cache and reapply the
uinput mouse range for the new layout. The refresh runs off the frame-receive
loop and is coalesced across the per-display receivers, so a multi-monitor
hotplug runs one worker and the final layout wins.
- Clear the prefer-CPU-convert hints on a topology change: display indices can
be renumbered, so a hint learned for an old index no longer refers to the same
physical display. Re-learned on the next convert failure.
- Report a non-DRM-backed display when the DRM list is shorter than the sync
list or any entry is offline, covering the present-but-demoted case.
Point the DRM capture build at the libdrmtap v0.4.14 release commit
(816766dedaba3140c613712ce97aa2614e8899e7) instead of v0.4.13, in build.py and
the flutter-build workflow, and correct the scrap Cargo.toml note to describe
the actual DRMTAP_SHA anchor. 0.4.14 keeps the same public API, so the dlopen
consumer needs no change.
The _drm unit tests are behind the opt-in drm feature, which the default workspace
test job does not build, so they would sit in the tree unrun -- no better than no
tests. Add a Linux step to the per-PR ci.yml that runs them with the feature on,
alongside the existing ipc/auth tests. drm is a pure runtime-dlopen backend with no
link-time deps (no libdrm/EGL/gbm) and the tests are pure userspace (socketpair
framing, SCM_RIGHTS, the peer-auth/admission decisions), so this needs no GPU and no
extra system packages. The main build/test stays on default features, so the shipped
drm-off config remains the primary verified one.
5e: the deb dropped /usr/lib/rustdesk into /etc/ld.so.conf.d so the private libdrmtap
could be found by soname -- a system-wide search-path entry that lets it shadow a
system library for every binary on the host, which Debian Policy 10.2 forbids. Resolve
it by absolute path (/usr/lib/rustdesk/libdrmtap.so.0) at the dlopen site instead, with
the bare sonames kept only as a dev fallback, and drop the ld.so.conf.d file and the
ldconfig/try-restart postinst entirely (the .so is present at its absolute path right
after unpack, so the pre-warm resolves with no linker-cache step). The dlopen site is
this PR's own code, so this is in scope, not a follow-up.
6a: extract the _drm admission bound and the per-frame re-auth decision into pure
helpers (drm_conn_admitted, drm_peer_authorized) and unit-test them: admission admits
strictly below MAX_DRM_CONNS and rejects at/above it; re-auth passes root always,
passes a non-root peer only while it equals the active-session uid, and fails closed on
a switched-away, unknown-session, or unknown-peer case. (The /proc/exe-mismatch
rejection is exercised by the accept-time authorize call; unit-testing it in isolation
would need a second process with a different exe, so it stays an integration concern.)
The dlopened .so is loaded into the CAP_SYS_ADMIN root service, so it should come
from the maintainer-owned repo, not a personal fork. rustdesk-org/libdrmtap main is
already synced to the exact commit we pin (c9cf0938 = v0.4.13) but carries no release
tag, so point both build.py and the CI job at rustdesk-org and track main with the
immutable commit pinned via DRMTAP_SHA. The post-clone sha check makes this
fail-closed: main moving off the pinned commit fails the build instead of silently
swapping the .so. The CI ref guard now accepts a vX.Y.Z tag or main (a loose branch is
still rejected). Switch DRMTAP_REF to a tag if rustdesk-org later publishes one.
The cold availability probe in is_available acquired DRM_PROBE_IN_FLIGHT and released
it with a plain store(false) after a synchronous body; a panic there (e.g. a poisoned
DRM_STATE lock) would leak the guard true and freeze both future probes and the
refresh path hardened in the previous commit, since they share the guard. Hoist the
release into a shared ProbeInFlightGuard used by both the cold probe and the refresh
closure, so any exit -- normal, early, or unwinding -- clears it.
Five defects an adversarial pass found in the previous commits:
- refresh_available_async set the single-flight probe guard, then relied on the
detached thread to clear it; if thread creation failed (EAGAIN) or the closure
unwound, the guard leaked true and froze every future probe. Release it via RAII
inside the closure and on a Builder::spawn error.
- The _drm per-frame re-auth called the cached active_uid(), which on a cache miss
(exactly during a session switch) falls back to a blocking loginctl seat0 lookup --
on the single-threaded _drm runtime, once per frame, a subprocess storm. Use a new
cache-only accessor that never blocks and fails closed on a miss, and correct the
comment: the stop is bounded by the active-uid cache cadence, not one frame.
- set_drm_cursor inserted unconditionally, so a still-draining predecessor stream
could overwrite (then delete on teardown) the cursor a replacement stream published
for the same index. Make it a compare-and-set that ignores an older epoch.
- recv_msg_timeout2 treated a spurious readable() wakeup with nothing consumed as a
mid-frame stall and tore the stream down. Track whether any byte was consumed
(drm_read_full sets it) and map a zero-progress deadline back to None (re-poll),
reserving the hard error for a genuine partial-frame stall.
The _drm wire format is hand-rolled (length prefix plus an fd bound to the frame
first byte) because Framed/BytesCodec cannot carry ancillary data, so it had zero
tests. Add pure-userspace coverage over a socketpair:
- a control message round-trips with and without an attached fd, and the received fd
refers to the same open file (a byte written into the source is read back through it)
- a raw length-prefixed body (cursor / CPU-fallback path) round-trips byte-for-byte
- a forged length prefix past the JSON cap is rejected at the prefix
- surplus fds packed into one cmsg keep only the first and close the rest
- a control message truncated past DRM_CMSG_CAP is rejected (MSG_CTRUNC), not consumed
- peer_uid_from_fd reads the socket peer credential the producer-auth path relies on
- recv_msg_timeout2 only gated the wait for the first byte, so a peer that sent one
byte then stalled pinned the task forever. The same budget now also bounds the body
read; a body that overruns is a hard error that tears the stream down (recv_msg
bodies are small JSON, so a healthy peer never trips it).
- The cursor cache is keyed by display index, which a rebuilt stream reuses, so a
predecessor exiting after its replacement published a fresh cursor erased it. Stamp
each entry with a monotonic per-stream epoch and compare-and-remove on teardown.
- ProbeState::Available had no TTL, so an idle hotplug left a phantom display in
enumeration. Give it a timestamp and refresh the list off the hot path once it ages
past POSITIVE_TTL. The verdict stays true across the refresh (never bounces a live
session to the portal) and the probe runs on a background thread (never blocks the
async enumeration).
- Remove the dead clear(): it is unreferenced, and wiring it into teardown would force
the blocking re-probe on the next enumeration that swap_available_displays exists to
avoid.
