From 584fac0c6da5a6f61a823367739e3ab12d42496b Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Wed, 29 Jul 2026 21:12:48 -0300 Subject: [PATCH] drm: the same latched-flag bug a fourth time, in my own fix for the third I built UinputRefreshGuard INSIDE the spawned closure, so it only covered paths where the closure ran. thread::spawn panics on EAGAIN after the swap, so no guard existed and the flag stayed set for the process lifetime, which is the exact failure the guard was introduced to prevent. I then wrote RefreshSlot correctly - constructed before the spawn, moved in - two hours later and did not go back to fix its sibling. Both are right now, and the spawn is fallible in both. Also from the review: - DRMTAP_PREBUILT_DIR returned before the EGL-stub assertion, so the check only guarded the source build. That is backwards: prebuilt-dir is the widest override (no fetch, no sha check, an object this script never sees), the likeliest to hand over a stub, and the path our aarch64 cross-build actually uses. Verified the assertion accepts a real .so and rejects one built with -Degl=disabled. - convert() bounded only the frame libdrmtap returns, not the descriptor going in. offsets/pitches address plane ranges inside the dma-buf, so those are what a malformed pair would reach past. Bounded per populated plane, the same way the export side is. Defense in depth (the producer is root-authenticated and libdrmtap validates against the fd since 0.4.12), but the two halves should agree before the C sees the data, not after. - the flutter patch step used '[[ test ]] && git apply' as its last command, so the step would FAIL rather than skip the first time FLUTTER_VERSION moves off 3.24.5. Explicit if/else, and the values now come from the environment instead of ${{ }} interpolation, which also clears zizmor's template-injection warning. Checked both branches. Declined: the cursor id/cache-key convergence finding. Both accessors use one selection over one map, so they can only disagree across a publish race, and state.hcursor is already set to the id ACTUALLY served (drm_served_id), which is the sync the finding asks for - added in an earlier round. --- .github/workflows/drm-capture.yml | 10 +++++++- build.py | 9 ++++++- libs/scrap/src/common/drm_render.rs | 37 +++++++++++++++++++++++++++++ src/server/drm_capturer.rs | 19 +++++++++++---- 4 files changed, 69 insertions(+), 6 deletions(-) diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index fa4d47b9e..372acf2bf 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -278,7 +278,15 @@ jobs: shell: bash run: | cd $(dirname $(dirname $(which flutter))) - [[ "3.24.5" == ${{ env.FLUTTER_VERSION }} ]] && git apply ${{ github.workspace }}/.github/patches/flutter_3.24.4_dropdown_menu_enableFilter.diff + # `[[ ... ]] && cmd` as the last line makes the STEP fail once FLUTTER_VERSION moves off + # the pinned value, because the failed test becomes the script's exit status. An explicit + # if/else skips instead. Reading the values from the environment rather than interpolating + # ${{ }} into the script also keeps this off zizmor's template-injection list. + if [[ "$FLUTTER_VERSION" == "3.24.5" ]]; then + git apply "$GITHUB_WORKSPACE/.github/patches/flutter_3.24.4_dropdown_menu_enableFilter.diff" + else + echo "::notice::flutter $FLUTTER_VERSION is not 3.24.5; skipping the dropdown patch" + fi - name: Build the unattended-wayland deb shell: bash diff --git a/build.py b/build.py index 14a258b3e..1d3a5158d 100755 --- a/build.py +++ b/build.py @@ -409,7 +409,14 @@ def build_libdrmtap_so(): # DRMTAP_PREBUILT_DIR explicitly names the artifact source, so honor it strictly: fail # (rather than silently falling back to a source build) if it holds no single real .so. prebuilt = glob.glob(os.path.join(prebuilt_dir, 'libdrmtap.so.0.*')) - return _single_real_so(prebuilt, f'DRMTAP_PREBUILT_DIR={prebuilt_dir}') + so = _single_real_so(prebuilt, f'DRMTAP_PREBUILT_DIR={prebuilt_dir}') + # Check the stub case HERE too, not only on the source path below. This is the widest + # override of the three -- no fetch, no sha verification, an object built by something this + # script cannot see -- so it is the likeliest to hand over a CPU-only build, and skipping the + # assertion on exactly this path would leave the check guarding only the case that was + # already trustworthy. + _assert_so_has_egl(so) + return so # Fetch the pinned source if it is not already present. third_party/libdrmtap is not a submodule # anymore; it is git-ignored. The commit is fetched BY SHA rather than by cloning a branch: # `clone --depth 1 --branch main` only ever fetches the tip, so the moment upstream pushes to diff --git a/libs/scrap/src/common/drm_render.rs b/libs/scrap/src/common/drm_render.rs index 1218de1ba..e5f89a602 100644 --- a/libs/scrap/src/common/drm_render.rs +++ b/libs/scrap/src/common/drm_render.rs @@ -116,6 +116,43 @@ impl RenderConverter { desc: &mut drmtap_dmabuf_desc, received_fd: RawFd, ) -> io::Result<(&[u8], u32, u32, Pixfmt)> { + // Bound the INCOMING descriptor, not just the frame libdrmtap hands back. Every field here + // was deserialized from the `_drm` wire, and while the producer is authenticated as root + // (`connect_drm` refuses a non-root peer) and libdrmtap has validated the fd against the + // descriptor since 0.4.12, this side already bounds the export descriptor symmetrically -- + // the two halves of the split should agree about what they will touch before the C sees it, + // not after. + // + // offsets/pitches are the interesting ones: they address plane ranges inside the dma-buf, so + // a malformed pair is what would reach past the buffer. Bound each populated plane's extent + // the same way the frame path is bounded. + { + let (w, h) = (desc.width, desc.height); + if w == 0 || h == 0 || w > MAX_DIM || h > MAX_DIM { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("drm: refusing a dma-buf descriptor with geometry {w}x{h}"), + )); + } + let planes = desc.num_planes.clamp(1, 4) as usize; + for p in 0..planes { + let extent = (desc.pitches[p] as usize) + .checked_mul(h as usize) + .and_then(|rows| rows.checked_add(desc.offsets[p] as usize)); + match extent { + Some(end) if end <= MAX_FRAME_BYTES => {} + other => { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "drm: refusing dma-buf plane {p} (offset {} pitch {} over {h} rows -> {other:?}, cap {MAX_FRAME_BYTES})", + desc.offsets[p], desc.pitches[p] + ), + )); + } + } + } + } // Always bound: a libdrmtap without the split convert symbols never loads. let convert_dmabuf = self.lib.convert_dmabuf; // Overwrite the descriptor's fd with the one THIS process received (split_capture.c diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 4b1ddfa15..9801bdfcc 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -797,10 +797,15 @@ async fn recv_thread( // runs ONE thread and the final layout wins. Not ordered against frame delivery. 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); + // Take ownership of the flag BEFORE the spawn and move it into the closure. + // Constructing the guard inside the closure only covers paths where the closure + // actually runs: `thread::spawn` panics on EAGAIN, and that happens after the + // swap above, so no guard would ever exist and the flag would stay set for the + // process lifetime. Same mistake, same shape, as the two flags before it. + let mut busy = UinputRefreshGuard(true); + let spawned = std::thread::Builder::new() + .name("drm-uinput-refresh".into()) + .spawn(move || { let rt = match tokio::runtime::Builder::new_current_thread() .enable_all() .build() @@ -835,6 +840,12 @@ async fn recv_thread( } } }); + if let Err(err) = spawned { + // The closure never ran, so it was dropped along with the guard it owned, + // and the guard released the slot: the next topology change retries the + // spawn instead of being locked out for the process lifetime. + log::error!("drm: could not spawn the uinput refresh worker: {err}"); + } } } _ => {} // ignore any unexpected control message