drm: never latch the uinput refresh slot, and bound the source stride

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.
This commit is contained in:
Mariano Abad
2026-07-28 23:25:18 -03:00
parent d66c2c1b78
commit cff25dc4d8
3 changed files with 58 additions and 4 deletions

View File

@@ -63,6 +63,7 @@ jobs:
drm-tests:
name: drm unit tests (linux)
runs-on: ubuntu-24.04
timeout-minutes: 60
steps:
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1
@@ -131,6 +132,7 @@ jobs:
libdrmtap:
name: libdrmtap pin, build and .so contract
runs-on: ubuntu-24.04
timeout-minutes: 60
steps:
- name: Checkout source code
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
@@ -199,6 +201,7 @@ jobs:
name: unattended-wayland deb (verification build)
needs: generate-bridge
runs-on: ubuntu-24.04
timeout-minutes: 60
steps:
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1

View File

@@ -256,6 +256,24 @@ impl DrmReader {
));
}
};
// Bound the SOURCE extent too, not just the destination. The row loop below reads up to
// (h-1)*stride + w*4, so a large stride reads far past the mapping however small the
// destination is, and `y * stride` can overflow usize on the way. drm_render::convert
// bounds stride*h the same way; the two halves of the split must agree about what they
// are willing to touch, or the privileged half is the weaker one.
match stride.checked_mul(h) {
Some(sz) if sz > 0 && sz <= MAX_FRAME_BYTES => {}
other => {
log::warn!(
"DRM scanout stride {stride} x {h} rows is out of range ({other:?} bytes); falling back"
);
(self.lib.frame_release)(self.ctx, &mut frame);
return Err(io::Error::new(
io::ErrorKind::Other,
"DRM scanout stride out of range",
));
}
}
if self.buf.len() != frame_size {
self.buf.resize(frame_size, 0);
}

View File

@@ -798,6 +798,9 @@ async fn recv_thread(
UINPUT_REFRESH_GEN.fetch_add(1, Ordering::AcqRel);
if !UINPUT_REFRESH_BUSY.swap(true, Ordering::AcqRel) {
std::thread::spawn(|| {
// We hold the flag from the swap above; the guard hands it back on every
// exit from here, including a panic inside the refresh below.
let mut busy = UinputRefreshGuard(true);
let rt = match tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
@@ -810,8 +813,7 @@ async fn recv_thread(
log::warn!(
"drm: uinput refresh worker could not build a runtime: {err}"
);
UINPUT_REFRESH_BUSY.store(false, Ordering::Release);
return;
return; // the guard hands the slot back
}
};
let mut served = 0u64;
@@ -824,11 +826,11 @@ async fn recv_thread(
}
// Caught up: release, then re-check for a request that raced in after our
// load but before the release, taking the worker role back if so.
UINPUT_REFRESH_BUSY.store(false, Ordering::Release);
busy.release();
if UINPUT_REFRESH_GEN.load(Ordering::Acquire) == served {
break;
}
if UINPUT_REFRESH_BUSY.swap(true, Ordering::AcqRel) {
if !busy.retake() {
break; // another handler already started a fresh worker
}
}
@@ -1031,6 +1033,37 @@ impl Drop for ProbeInFlightGuard {
}
}
/// Ownership of `UINPUT_REFRESH_BUSY`, released on every exit including an unwind. Same job as
/// `ProbeInFlightGuard`, but this flag is deliberately handed back and re-taken mid-loop (the
/// caught-up/re-check dance), so it has to track whether we still hold it: releasing on drop
/// unconditionally would clear a flag a REPLACEMENT worker owns.
///
/// Without this the worker body could leave it latched true forever -- it locks several process-wide
/// mutexes and does a Wayland roundtrip -- and every later hotplug would then skip the spawn and
/// never reapply the uinput range, which is the stale-range/wrong-output symptom
/// `wayland::update_uinput_resolution` is written to prevent.
struct UinputRefreshGuard(bool);
impl UinputRefreshGuard {
/// Hand the flag back, if we are the ones holding it.
fn release(&mut self) {
if self.0 {
self.0 = false;
UINPUT_REFRESH_BUSY.store(false, Ordering::Release);
}
}
/// Take the worker role back. False when someone else already did, in which case we are NOT the
/// owner and must not release it on the way out.
fn retake(&mut self) -> bool {
self.0 = !UINPUT_REFRESH_BUSY.swap(true, Ordering::AcqRel);
self.0
}
}
impl Drop for UinputRefreshGuard {
fn drop(&mut self) {
self.release();
}
}
/// Whether the root service offers DRM/KMS capture. The positive result and a definitive negative
/// (connected, but no displays) are cached; a transient probe error stays `Unknown` for a few
/// retries. Normally the cache is warmed at `--server` startup (`warm_availability`), so the first