diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index c058770e6..d9b610d57 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -215,8 +215,22 @@ jobs: echo "::error::extracted only $nsyms loader symbols from drmtap_dl.rs (expected >= 13); the extraction pattern no longer matches the loader" missing=1 fi + # Inspect the object ONCE into a variable, then match with bash's own pattern operator -- + # NO PIPE ANYWHERE IN THESE CHECKS. `anything | grep -q` under `set -o pipefail` reports a + # FALSE FAILURE as soon as the producer outruns the 64 KB pipe buffer: grep -q exits at the + # first match, the producer dies on SIGPIPE (141), and pipefail makes that the pipeline's + # status, so a library that HAS the symbol is reported as missing it. Measured on a real + # EGL-enabled .so (101 KB of `strings`, both markers present): the piped form reported both + # missing and failed the step. Note the obvious repair does NOT work -- materializing the + # output and then doing `printf '%s\n' "$var" | grep -q` keeps the pipe and just swaps the + # producer, and it fails identically (measured). Today's release-sized .so happens to fit in + # the buffer, which is the only reason this has not fired yet. + exported="$(nm -D --defined-only "$SO")" + strs="$(strings "$SO")" for sym in $syms; do - if ! nm -D --defined-only "$SO" | grep -q " T $sym\$"; then + # Line-anchored: wrap in newlines so the pattern can require a whole line, the same + # thing `grep " T $sym$"` was expressing. + if [[ $'\n'"$exported"$'\n' != *$'\n'*" T $sym"$'\n'* ]]; then echo "::error::libdrmtap does not export $sym, which the runtime loader resolves" missing=1 fi @@ -226,7 +240,7 @@ jobs: # for: the naive ELF check reports "no EGL" on a perfectly good library. What a CPU-only stub # build really lacks is the dlopen target name and the import call itself. for s in "libEGL.so.1" "eglCreateImageKHR"; do - if ! strings "$SO" | grep -qF "$s"; then + if [[ "$strs" != *"$s"* ]]; then echo "::error::libdrmtap looks like a CPU-only stub (no $s): the EGL detile hot path is missing" missing=1 fi @@ -346,8 +360,19 @@ jobs: fi deb="${debs[0]}" echo "::notice::built $deb ($(stat -c %s "$deb") bytes)" - dpkg -c "$deb" | grep -E 'usr/lib/rustdesk/libdrmtap\.so\.0\.[0-9]+\.[0-9]+$' - dpkg -c "$deb" | grep -E 'usr/lib/rustdesk/libdrmtap\.so\.0 ->' + # Pipe-free for the same reason as the .so contract step above (see the comment there: + # a producer feeding a grep that can exit early is a SIGPIPE reported as a failure under + # pipefail). `grep -E` without -q reads to EOF so these two happen to be safe, but the + # shape is the hazard and the next `-q` added here would inherit it silently. + contents="$(dpkg -c "$deb")" + if [[ ! "$contents" =~ usr/lib/rustdesk/libdrmtap\.so\.0\.[0-9]+\.[0-9]+ ]]; then + echo "::error::the deb does not contain a versioned libdrmtap.so.0.x.y" + exit 1 + fi + if [[ "$contents" != *"usr/lib/rustdesk/libdrmtap.so.0 ->"* ]]; then + echo "::error::the deb does not contain the libdrmtap.so.0 soname symlink" + exit 1 + fi # The library alone does not make this a drm build: build.py stages it whenever --drm is # passed, independently of what was compiled, and the deb name is what tells a user this # is the consent-bypass variant. Assert the BINARY too, by the absolute dlopen path that diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 4f9e5bf0b..65863976d 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -365,8 +365,14 @@ impl DrmReader { /// libdrmtap that does not export this symbol (see `drmtap_dl::abi_accepted`). pub fn grab_desc(&mut self) -> io::Result<(OwnedFd, drmtap_dmabuf_desc)> { let grab_desc = self.lib.grab_desc; - // SAFETY: self.ctx is a valid context; desc/frame are zeroed before the - // call and the frame is released on every return path (after the dup). + // SAFETY: self.ctx is a valid context; desc/frame are zeroed before the call. The frame is + // released on every return path that OWNS one, which is not the same as every return path: + // a failing `drmtap_grab_desc` leaves nothing for us to release, and releasing anyway would + // be a double free. Traced in the C rather than assumed -- on `-EINVAL` it returns before + // allocating, on a failed inner grab that grab has already cleaned up after itself, and on + // `-ENOTSUP` (pixels but no transferable fd) libdrmtap calls `drmtap_frame_release` ITSELF + // before returning. So the error arm below deliberately returns without releasing, and only + // the paths that reach a populated frame release it, after dupping the fd out of it. unsafe { let mut desc: drmtap_dmabuf_desc = std::mem::zeroed(); let mut frame: drmtap_frame_info = std::mem::zeroed(); diff --git a/src/server.rs b/src/server.rs index be62e74d0..4029ed453 100644 --- a/src/server.rs +++ b/src/server.rs @@ -603,12 +603,15 @@ 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. + // X11 is skipped -- probing there makes the root service open DRM readers for a path this + // session can never take -- but that decision belongs to `warm_availability`, which already + // makes it, and NOT to this call site. Deciding it here is the same one-shot-at-startup + // mistake the pre-warm had: `is_x11()` answers "x11" whenever loginctl cannot yet name the + // seat0 session, which during a boot is exactly when this runs, and nothing revisits it -- + // so a Wayland host that came up slowly skipped the warm for the life of the process and + // got back the cold-probe "No displays" symptom the warm exists to remove. #[cfg(all(target_os = "linux", feature = "drm"))] - if !scrap::is_x11() { - std::thread::spawn(drm_capturer::warm_availability); - } + std::thread::spawn(drm_capturer::warm_availability); input_service::fix_key_down_timeout_loop(); #[cfg(target_os = "linux")] if input_service::wayland_use_uinput() { diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 81f266de2..1ba913e59 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -1411,10 +1411,28 @@ pub(super) fn warm_availability() { // Nothing on X11 can consume a DRM stream, and probing makes the ROOT service open DRM readers, // so an X11 host running a drm build would pay that at every startup for a path it can never // take. The lazy probe behind is_available is reached only from the Wayland paths already. - if crate::platform::linux::is_x11() { - return; - } + // + // Two things about WHERE and HOW this is decided, both learned the hard way on the pre-warm's + // identical gate (see `drm_prewarm` in ipc/drm.rs): + // + // - it is decided HERE, not at the call site in server.rs. This runs during startup, and + // `get_display_server()` answers "x11" whenever loginctl cannot yet name the seat0 session, + // which during a boot is precisely then. A one-shot check outside the retry loop skipped the + // warm for the life of the process on a Wayland host that came up slowly, handing back the + // cold-probe "No displays" symptom this function exists to remove. + // - it uses `scrap::is_x11()`, the UNMEMOISED form that re-runs loginctl per call. + // `crate::platform::linux::is_x11()` latches its first answer in a lazy_static, so asking it + // inside a retry loop would re-read the same early "x11" ten times and change nothing. + // + // The re-check rides the existing retry loop rather than adding a second wait: a genuine X11 + // host spends the same ten short attempts it already spent on `query_displays` and opens + // nothing, and a host whose session turns out to be Wayland proceeds on the attempt where that + // becomes knowable. for _ in 0..10 { + if scrap::is_x11() { + std::thread::sleep(Duration::from_millis(300)); + continue; + } if matches!(&*DRM_STATE.lock().unwrap(), ProbeState::Available(..)) { return; }