From 8a96d6795f0a31aa1ec550dec3f791c30abbc737 Mon Sep 17 00:00:00 2001 From: Mamdasan Sabrian Date: Mon, 17 Aug 2026 00:49:57 +0200 Subject: [PATCH 1/4] fix(linux): validate mapped DMA-BUF frames --- .../native/pipewire-capture/csrc/pw_shim.c | 179 ++++++++++++++++-- .../native/pipewire-capture/csrc/pw_shim.h | 5 + electron/native/pipewire-capture/src/main.rs | 2 +- electron/native/pipewire-capture/src/shim.rs | 175 ++++++++++++++++- 4 files changed, 337 insertions(+), 24 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 82a8a129e..e12c672f6 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -85,6 +85,11 @@ static int osc_debug_enabled(void); * rather than trusting that. */ #define OSC_MAX_DMABUF_MAPS 32 +#define OSC_FRAME_DROP_REPORTS 5 +#define OSC_VIDEO_BYTES_PER_PIXEL 4 +/* xdg-desktop-portal-wlr uses 9 as a sentinel when a DMA-BUF + * chunk size is unknown. */ +#define OSC_XDPW_DMABUF_SIZE_SENTINEL 9 struct osc_dmabuf_map { int fd; @@ -183,6 +188,7 @@ struct osc_pw_session { * decides whether buffers arrive as dmabuf fds or shared memory. */ int uses_dmabuf; struct osc_dmabuf_map dmabuf_maps[OSC_MAX_DMABUF_MAPS]; + int frame_drop_reports; /* fd whose DMA_BUF_SYNC_START has not been closed by its END yet, or -1. * The bracket has to span the on_frame callback, not just osc_read_frame, * because the callback is where the pixels are actually read. */ @@ -651,6 +657,7 @@ static void osc_on_param_changed(void *userdata, uint32_t id, const struct spa_p * with them. Re-arm the reports so the instrumentation describes the buffer * set actually in use rather than a set that no longer exists. */ session->buffer_info_reports = 0; + session->frame_drop_reports = 0; api.stream_update_params(session->stream, params, SPA_N_ELEMENTS(params)); } @@ -740,18 +747,145 @@ static void *osc_map_dmabuf(int fd, size_t *len, const char **why) return ptr == MAP_FAILED ? NULL : ptr; } -static void *osc_find_dmabuf_map(struct osc_pw_session *session, int fd) +static struct osc_dmabuf_map *osc_find_dmabuf_map(struct osc_pw_session *session, int fd) { size_t i; for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { if (session->dmabuf_maps[i].ptr != NULL && session->dmabuf_maps[i].fd == fd) { - return session->dmabuf_maps[i].ptr; + return &session->dmabuf_maps[i]; } } return NULL; } +enum osc_frame_bounds_error { + OSC_FRAME_BOUNDS_OK, + OSC_FRAME_BOUNDS_NO_CAPACITY, + OSC_FRAME_BOUNDS_OFFSET, + OSC_FRAME_BOUNDS_CORRUPTED, + OSC_FRAME_BOUNDS_GEOMETRY, + OSC_FRAME_BOUNDS_ROW_TOO_SHORT, + OSC_FRAME_BOUNDS_FRAME_TOO_LARGE, +}; + +static enum osc_frame_bounds_error osc_resolve_frame_bounds( + uint32_t data_type, uint32_t maxsize, size_t mapped_len, uint32_t chunk_offset, + uint32_t chunk_size, int32_t chunk_flags, int32_t stride, int32_t width, int32_t height, + size_t *available_out, size_t *offset_out, size_t *size_out) +{ + size_t available; + size_t offset; + size_t size; + uint64_t row_bytes; + uint64_t frame_bytes; + + /* PipeWire maps MemPtr/MemFd for us and maxsize is their allocation bound. + * DMA-BUF is mapped by osc_on_add_buffer, which may have recovered a real + * length from the fd when the producer legitimately left maxsize at zero. */ + available = data_type == SPA_DATA_DmaBuf ? mapped_len : (size_t)maxsize; + *available_out = available; + if (available == 0) { + return OSC_FRAME_BOUNDS_NO_CAPACITY; + } + offset = chunk_offset; + if (offset > available) { + return OSC_FRAME_BOUNDS_OFFSET; + } + if ((chunk_flags & SPA_CHUNK_FLAG_CORRUPTED) != 0) { + return OSC_FRAME_BOUNDS_CORRUPTED; + } + if (data_type == SPA_DATA_DmaBuf && maxsize == 0 && + chunk_size == OSC_XDPW_DMABUF_SIZE_SENTINEL) { + /* Use the mapped length for xdpw's unknown-size sentinel. */ + size = available - offset; + } else { + /* Validate offset first so this subtraction cannot underflow. */ + size = SPA_MIN((size_t)chunk_size, available - offset); + } + if (stride <= 0 || width <= 0 || height <= 0) { + return OSC_FRAME_BOUNDS_GEOMETRY; + } + row_bytes = (uint64_t)width * OSC_VIDEO_BYTES_PER_PIXEL; + if ((uint64_t)stride < row_bytes) { + return OSC_FRAME_BOUNDS_ROW_TOO_SHORT; + } + /* Widen before multiplying: both operands originate outside this process. */ + frame_bytes = (uint64_t)stride * (uint64_t)height; + if (frame_bytes > (uint64_t)size) { + return OSC_FRAME_BOUNDS_FRAME_TOO_LARGE; + } + *offset_out = offset; + *size_out = size; + return OSC_FRAME_BOUNDS_OK; +} + +int osc_pw_frame_bounds_valid(uint32_t data_type, uint32_t maxsize, size_t mapped_len, + uint32_t chunk_offset, uint32_t chunk_size, int32_t chunk_flags, + int32_t stride, int32_t width, int32_t height) +{ + size_t available; + size_t offset; + size_t size; + + return osc_resolve_frame_bounds(data_type, maxsize, mapped_len, chunk_offset, chunk_size, + chunk_flags, stride, width, height, &available, &offset, &size) == + OSC_FRAME_BOUNDS_OK; +} + +static const char *osc_frame_bounds_error_name(enum osc_frame_bounds_error error) +{ + switch (error) { + case OSC_FRAME_BOUNDS_NO_CAPACITY: + return "buffer-length-zero"; + case OSC_FRAME_BOUNDS_OFFSET: + return "chunk-offset-out-of-bounds"; + case OSC_FRAME_BOUNDS_CORRUPTED: + return "producer-marked-frame-corrupted"; + case OSC_FRAME_BOUNDS_GEOMETRY: + return "invalid-frame-geometry"; + case OSC_FRAME_BOUNDS_ROW_TOO_SHORT: + return "stride-shorter-than-row"; + case OSC_FRAME_BOUNDS_FRAME_TOO_LARGE: + return "frame-bytes-exceed-available-chunk"; + case OSC_FRAME_BOUNDS_OK: + return "none"; + } + return "unknown"; +} + +static const char *osc_data_type_name(uint32_t data_type) +{ + switch (data_type) { + case SPA_DATA_MemPtr: + return "MemPtr"; + case SPA_DATA_MemFd: + return "MemFd"; + case SPA_DATA_DmaBuf: + return "DmaBuf"; + default: + return "Unknown"; + } +} + +static void osc_report_frame_drop(struct osc_pw_session *session, const struct spa_data *data, + size_t mapped_len, size_t available, + enum osc_frame_bounds_error error) +{ + if (!osc_debug_enabled() || session->frame_drop_reports >= OSC_FRAME_DROP_REPORTS) { + return; + } + session->frame_drop_reports++; + fprintf(stderr, + "[osc-frame-drop] reason=%s data_type=%s data_type_id=%u fd=%lld maxsize=%u " + "mapped_len=%zu available_len=%zu chunk_offset=%u chunk_size=%u " + "chunk_flags=%d stride=%d width=%u height=%u format=%u\n", + osc_frame_bounds_error_name(error), osc_data_type_name(data->type), data->type, + (long long)data->fd, data->maxsize, mapped_len, available, data->chunk->offset, + data->chunk->size, data->chunk->flags, data->chunk->stride, session->format.size.width, + session->format.size.height, session->format.format); +} + /* * CPU access to a dmabuf has to be bracketed by DMA_BUF_IOCTL_SYNC, or the * driver is under no obligation to have flushed the GPU's writes into the @@ -969,20 +1103,25 @@ static int osc_read_cursor(const struct spa_buffer *buffer, struct osc_pw_cursor * Extracts the pixels of one buffer. Returns 1 when `out` describes a frame, 0 * when this buffer carries none. * - * The offset/size clamping against `maxsize` is the standard PipeWire consumer - * idiom and is not paranoia: `chunk` lives in memory the PRODUCER writes, so its - * fields are untrusted input from another process. A compositor bug — or a - * malicious one — that reports a size past the end of the mapping would - * otherwise be a read straight off the end of the shared memory. + * The offset/size clamping against the mapped length is the standard PipeWire + * consumer idiom and is not paranoia: `chunk` lives in memory the PRODUCER + * writes, so its fields are untrusted input from another process. A compositor + * bug — or a malicious one — that reports a size past the end of the mapping + * would otherwise be a read straight off the end of the shared memory. For + * DMA-BUF, that length comes from dmabuf_maps[] because maxsize may be zero. */ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffer *buffer, struct osc_pw_frame *out) { struct spa_data *data; + struct osc_dmabuf_map *dmabuf_map = NULL; struct spa_meta_header *header; struct spa_meta_region *region; - uint32_t offset; - uint32_t size; + enum osc_frame_bounds_error bounds_error; + size_t available; + size_t mapped_len; + size_t offset; + size_t size; int32_t stride; int32_t height; @@ -1006,10 +1145,12 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe * osc_on_add_buffer. A miss means the mmap failed there — reported at * that point — and there is nothing readable here. */ - base = osc_find_dmabuf_map(session, (int)data->fd); - if (base == NULL) { + dmabuf_map = osc_find_dmabuf_map(session, (int)data->fd); + if (dmabuf_map == NULL) { return 0; } + base = dmabuf_map->ptr; + mapped_len = dmabuf_map->len; } else if (data->data == NULL) { /* * NULL on a shared-memory buffer means it was never mapped, which is the @@ -1018,6 +1159,7 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe return 0; } else { base = data->data; + mapped_len = data->maxsize; } /* A zero-sized chunk is how a compositor ships a cursor update with no new * frame attached. Not an error, just not a frame. */ @@ -1025,17 +1167,14 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe return 0; } - offset = SPA_MIN(data->chunk->offset, data->maxsize); - size = SPA_MIN(data->chunk->size, data->maxsize - offset); - height = (int32_t)session->format.size.height; stride = data->chunk->stride; - if (stride <= 0 || height <= 0) { - return 0; - } - /* One short row is one row of garbage in the recording; refuse the whole - * frame instead, and let the caller count it as dropped. */ - if ((uint64_t)stride * (uint64_t)height > (uint64_t)size) { + bounds_error = osc_resolve_frame_bounds( + data->type, data->maxsize, mapped_len, data->chunk->offset, data->chunk->size, + data->chunk->flags, stride, (int32_t)session->format.size.width, height, &available, &offset, + &size); + if (bounds_error != OSC_FRAME_BOUNDS_OK) { + osc_report_frame_drop(session, data, mapped_len, available, bounds_error); return 0; } diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index ab6f78305..70a90ab14 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -171,6 +171,11 @@ int osc_pw_cursor_meta_accepts_producer_size(uint32_t width, uint32_t height); */ int osc_pw_enum_format_accepts_dmabuf_producer(int with_modifier, int64_t producer_modifier); +/* Test-only frame-bound validation without a live PipeWire buffer. */ +int osc_pw_frame_bounds_valid(uint32_t data_type, uint32_t maxsize, size_t mapped_len, + uint32_t chunk_offset, uint32_t chunk_size, int32_t chunk_flags, + int32_t stride, int32_t width, int32_t height); + struct osc_pw_session; /* diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index 1576efced..7f52747f4 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -940,7 +940,7 @@ fn run( ("metas", metas.clone().into()), ]), }); - if !has_cursor_meta { + if !has_cursor_meta && config.cursor_mode.reports_cursor() { let _ = emitter.emit(&Event::Warning { code: "no-cursor-metadata".to_owned(), message: format!( diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 8a4806ca4..f3057fb6b 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -102,6 +102,18 @@ extern "C" { with_modifier: i32, producer_modifier: i64, ) -> i32; + #[cfg(test)] + fn osc_pw_frame_bounds_valid( + data_type: u32, + maxsize: u32, + mapped_len: usize, + chunk_offset: u32, + chunk_size: u32, + chunk_flags: i32, + stride: i32, + width: i32, + height: i32, + ) -> i32; fn osc_pw_start( fd: i32, node_id: u32, @@ -611,6 +623,34 @@ pub fn enum_format_accepts_dmabuf_producer(with_modifier: bool, producer_modifie unsafe { osc_pw_enum_format_accepts_dmabuf_producer(i32::from(with_modifier), producer_modifier) } } +#[cfg(test)] +fn frame_bounds_valid( + data_type: u32, + maxsize: u32, + mapped_len: usize, + chunk_offset: u32, + chunk_size: u32, + chunk_flags: i32, + stride: i32, + width: i32, + height: i32, +) -> bool { + // SAFETY: the C helper performs arithmetic only and owns its output storage. + unsafe { + osc_pw_frame_bounds_valid( + data_type, + maxsize, + mapped_len, + chunk_offset, + chunk_size, + chunk_flags, + stride, + width, + height, + ) != 0 + } +} + /// SPA enum values as compiled from the vendored headers. pub fn constants() -> Constants { let mut out = Constants::default(); @@ -747,9 +787,9 @@ extern "C" fn on_frame(user: *mut c_void, frame: *const RawFrame) { if rows > frame.size { return; } - // SAFETY: the shim clamped `size` against the mapping's `maxsize` before - // the callback, `rows <= size` was just checked, and the mapping stays - // live until this returns. + // SAFETY: the shim clamped `size` against the mapping length before the + // callback, `rows <= size` was just checked, and the mapping stays live + // until this returns. let pixels = unsafe { std::slice::from_raw_parts(frame.data, rows) }; mailbox.put(pixels, frame); (state.sink)(StreamEvent::FrameReady); @@ -929,6 +969,135 @@ mod tests { ); } + #[test] + fn dmabuf_zero_maxsize_uses_recovered_mapping_length() { + let constants = constants(); + let stride = 1920 * 4; + + assert!(frame_bounds_valid( + constants.data_dma_buf, + 0, + 8 * 1024 * 1024, + 0, + 9, + 0, + stride as i32, + 1920, + 1080, + )); + } + + #[test] + fn frame_bounds_reject_invalid_offsets_and_geometry_without_affecting_memfd() { + let constants = constants(); + let stride = 16; + let frame_len = stride * 4; + + assert!(frame_bounds_valid( + constants.data_dma_buf, + frame_len, + frame_len as usize, + 0, + frame_len, + 0, + stride as i32, + 4, + 4, + )); + assert!(!frame_bounds_valid( + constants.data_dma_buf, + frame_len, + frame_len as usize, + frame_len + 1, + frame_len, + 0, + stride as i32, + 4, + 4, + )); + // An oversized chunk is capped to the remaining allocation, as SPA + // requires, and remains valid only when a whole frame still fits. + assert!(frame_bounds_valid( + constants.data_dma_buf, + frame_len, + frame_len as usize, + 0, + u32::MAX, + 0, + stride as i32, + 4, + 4, + )); + // Shared-memory buffers continue to use PipeWire's maxsize; a separate + // mapped length is meaningful only for DMA-BUF. + assert!(frame_bounds_valid( + constants.data_mem_fd, + frame_len, + 0, + 0, + frame_len, + 0, + stride as i32, + 4, + 4, + )); + assert!(!frame_bounds_valid( + constants.data_mem_fd, + frame_len, + 0, + 0, + frame_len, + 1, + stride as i32, + 4, + 4, + )); + assert!(!frame_bounds_valid( + constants.data_mem_fd, + 0, + frame_len as usize, + 0, + frame_len, + 0, + stride as i32, + 4, + 4, + )); + assert!(!frame_bounds_valid( + constants.data_dma_buf, + u32::MAX, + u32::MAX as usize, + 0, + u32::MAX, + 0, + i32::MAX, + 4, + i32::MAX, + )); + assert!(!frame_bounds_valid( + constants.data_dma_buf, + frame_len, + frame_len as usize, + 0, + frame_len, + 0, + 15, + 4, + 4, + )); + assert!(!frame_bounds_valid( + constants.data_dma_buf, + 0, + frame_len as usize, + 0, + 9, + 1, + stride as i32, + 4, + 4, + )); + } + /// End-to-end exercise of the PipeWire half with NO portal involved. /// /// `pw_context_connect_fd` accepts any socket already connected to a From e78f72fff893aa156d620d0de205f5d7483d673c Mon Sep 17 00:00:00 2001 From: Mamdasan Sabrian Date: Mon, 17 Aug 2026 19:16:42 +0200 Subject: [PATCH 2/4] fix(linux): trust DMA-BUF allocation size --- .../native/pipewire-capture/csrc/pw_shim.c | 37 +++++++++-------- electron/native/pipewire-capture/src/shim.rs | 40 +++++++++++++++++-- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index e12c672f6..047eaee69 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -87,9 +88,6 @@ static int osc_debug_enabled(void); #define OSC_MAX_DMABUF_MAPS 32 #define OSC_FRAME_DROP_REPORTS 5 #define OSC_VIDEO_BYTES_PER_PIXEL 4 -/* xdg-desktop-portal-wlr uses 9 as a sentinel when a DMA-BUF - * chunk size is unknown. */ -#define OSC_XDPW_DMABUF_SIZE_SENTINEL 9 struct osc_dmabuf_map { int fd; @@ -709,19 +707,21 @@ static void *osc_map_dmabuf(int fd, size_t *len, const char **why) return NULL; } /* - * A DmaBuf plane legitimately carries maxsize = 0: the size of a dmabuf is a - * property of the exporting buffer, not of the SPA descriptor, and wlroots - * leaves it unset. Every dmabuf fd is seekable to its own length, which is - * the documented way to recover it. Without this the mmap was never even - * attempted and the failure was reported as "this driver does not allow CPU - * mapping" — blaming the GPU for a size the producer simply had not filled in. + * A DmaBuf plane can carry an advisory maxsize rather than its allocation + * length. The allocation size is a property of the exporting fd, and every + * dmabuf fd is seekable to that length. Probe unconditionally and prefer a + * larger real allocation, while retaining a meaningful producer bound when + * the fd cannot report one. */ - if (*len == 0) { + { off_t probed = lseek(fd, 0, SEEK_END); - if (probed > 0) { + if (probed > 0 && (uintmax_t)probed <= SIZE_MAX && (size_t)probed > *len) { + size_t advertised = *len; *len = (size_t)probed; if (osc_debug_enabled()) { - fprintf(stderr, "[osc-dmabuf] maxsize=0, recovered %zu bytes via lseek\n", *len); + fprintf(stderr, + "[osc-dmabuf] maxsize=%zu, recovered %zu bytes via lseek\n", + advertised, *len); } } } @@ -781,8 +781,8 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( uint64_t frame_bytes; /* PipeWire maps MemPtr/MemFd for us and maxsize is their allocation bound. - * DMA-BUF is mapped by osc_on_add_buffer, which may have recovered a real - * length from the fd when the producer legitimately left maxsize at zero. */ + * DMA-BUF is mapped by osc_on_add_buffer, which recovers the real length + * from the fd when the producer leaves a placeholder in maxsize. */ available = data_type == SPA_DATA_DmaBuf ? mapped_len : (size_t)maxsize; *available_out = available; if (available == 0) { @@ -795,9 +795,12 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( if ((chunk_flags & SPA_CHUNK_FLAG_CORRUPTED) != 0) { return OSC_FRAME_BOUNDS_CORRUPTED; } - if (data_type == SPA_DATA_DmaBuf && maxsize == 0 && - chunk_size == OSC_XDPW_DMABUF_SIZE_SENTINEL) { - /* Use the mapped length for xdpw's unknown-size sentinel. */ + if (data_type == SPA_DATA_DmaBuf) { + /* DMA-BUF capacity belongs to the fd, not to SPA's advisory maxsize or + * chunk size. Backends use different positive placeholders for those + * fields, so keying this path on magic values is both brittle and + * unnecessary. The frame is still accepted only when stride * height + * fits inside the actual mapped allocation below. */ size = available - offset; } else { /* Validate offset first so this subtraction cannot underflow. */ diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index f3057fb6b..ab22f9eee 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -970,10 +970,12 @@ mod tests { } #[test] - fn dmabuf_zero_maxsize_uses_recovered_mapping_length() { + fn dmabuf_uses_the_mapped_allocation_not_advisory_sizes() { let constants = constants(); let stride = 1920 * 4; + // Portal backends use different placeholders for DMA-BUF sizes. None + // are special: the fd allocation and frame geometry are authoritative. assert!(frame_bounds_valid( constants.data_dma_buf, 0, @@ -985,6 +987,39 @@ mod tests { 1920, 1080, )); + assert!(frame_bounds_valid( + constants.data_dma_buf, + 1, + 8 * 1024 * 1024, + 0, + 1, + 0, + stride as i32, + 1920, + 1080, + )); + assert!(frame_bounds_valid( + constants.data_dma_buf, + 37, + 8 * 1024 * 1024, + 0, + 23, + 0, + stride as i32, + 1920, + 1080, + )); + assert!(!frame_bounds_valid( + constants.data_dma_buf, + u32::MAX, + 4096, + 0, + u32::MAX, + 0, + stride as i32, + 1920, + 1080, + )); } #[test] @@ -1015,8 +1050,7 @@ mod tests { 4, 4, )); - // An oversized chunk is capped to the remaining allocation, as SPA - // requires, and remains valid only when a whole frame still fits. + // DMA-BUF advisory sizes cannot extend the actual fd allocation. assert!(frame_bounds_valid( constants.data_dma_buf, frame_len, From 3fd31eee95a719581a110c1ac4ebc7144c867dbe Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Thu, 20 Aug 2026 17:32:52 +0000 Subject: [PATCH 3/4] fix(linux): close the diagnosis gaps in the DMA-BUF frame bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the frame-bounds change, from reviewing it. Each item below is a case where the code held the right answer and discarded it. The lseek probe now wins in BOTH directions. Preferring it only when it was LARGER left an over-declared maxsize as the mmap length, and the kernel refuses a dmabuf mapping longer than the object, so the import died reporting a driver that will not map — the exact misdiagnosis this path exists to prevent, with the correct length already in hand. A failed mmap now blames the right party. SPA_DATA_FLAG_MAPPABLE exists because "some memory types are not simply mappable (DmaBuf) unless explicitly specified with this flag", so when the producer left it unset it had already answered the question, and pointing at the GPU driver sends the reader somewhere else. chunk->size is weighed rather than trusted or ignored wholesale. Discarding it for every DMA-BUF buffer widened the accepted window to the whole allocation: at 1920x1080 in an 8 MiB mapping any stride from 7680 to 7767 was taken, and a half-written frame passed as a whole one. A chunk too small to hold one row is not a byte count and the mapping is the only bound left; anything at or above a row is believed. Placeholder backends (xdpw writes 9, niri writes 1) keep working, torn frames are refused again. The zero-chunk gate moved into the resolver. It sat in osc_read_frame alone, so the exported bound check accepted a buffer the reader silently refused — a contract the tests certified and the code did not implement. Frame drops reach the event stream. Every reason here is decided by values fixed for a whole negotiation, so a buffer that fails once fails every time and the recording comes out empty; a refused frame never reaches the mailbox, so frames-dropped stays at 0 and the session still stops "successfully". They went only to stderr behind OPENSCREEN_PIPEWIRE_DEBUG. That channel is new rather than borrowed. The dmabuf import failure used to ride on on_buffer_info with has_cursor_meta hardcoded to 0, so in the default cursor mode a capture that produced no frames at all was surfaced to the user as a cursor-metadata warning. Two silent failures now speak: a full mapping table (the loop fell through saying nothing on any channel, and every later lookup missed), and duplicate fds — several pw_buffers can be slices of one allocation, and the second mapping was a whole extra allocation that nothing would ever read. Tests: the exported helper reports WHICH bound rejected instead of a bare yes/no, so a reordering of the checks cannot keep the suite green, and the cases are built from a named baseline rather than nine positional integers. Adds the coverage that was missing entirely — the geometry gate on all six axes, MemPtr, and the empty chunk. Verified by mutation: reverting the chunk clamp, dropping either new geometry term, or removing the zero gate each turns a test red. Refs #287 Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01YFRtkHDpW15AfzCLtHN57A --- .../native/pipewire-capture/csrc/pw_shim.c | 305 +++++++++---- .../native/pipewire-capture/csrc/pw_shim.h | 35 +- electron/native/pipewire-capture/src/main.rs | 13 + electron/native/pipewire-capture/src/shim.rs | 426 +++++++++++------- 4 files changed, 510 insertions(+), 269 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 047eaee69..3c7452e39 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -121,7 +121,8 @@ struct osc_dmabuf_map { * unit test can assert instead of something a maintainer rediscovers. */ #define OSC_CURSOR_META_SIZE(w, h) \ - (sizeof(struct spa_meta_cursor) + sizeof(struct spa_meta_bitmap) + (size_t)(w) * (h) * 4) + (sizeof(struct spa_meta_cursor) + sizeof(struct spa_meta_bitmap) + \ + (size_t)(w) * (h) * OSC_VIDEO_BYTES_PER_PIXEL) /* * How many buffers to describe before going quiet. @@ -187,6 +188,7 @@ struct osc_pw_session { int uses_dmabuf; struct osc_dmabuf_map dmabuf_maps[OSC_MAX_DMABUF_MAPS]; int frame_drop_reports; + int capture_issue_reports; /* fd whose DMA_BUF_SYNC_START has not been closed by its END yet, or -1. * The bracket has to span the on_frame callback, not just osc_read_frame, * because the callback is where the pixels are actually read. */ @@ -656,6 +658,7 @@ static void osc_on_param_changed(void *userdata, uint32_t id, const struct spa_p * set actually in use rather than a set that no longer exists. */ session->buffer_info_reports = 0; session->frame_drop_reports = 0; + session->capture_issue_reports = 0; api.stream_update_params(session->stream, params, SPA_N_ELEMENTS(params)); } @@ -698,9 +701,10 @@ static int osc_debug_enabled(void) * fail are not interchangeable, and conflating them sent the one real * investigation of this path looking at the GPU driver for an hour. */ -static void *osc_map_dmabuf(int fd, size_t *len, const char **why) +static void *osc_map_dmabuf(int fd, uint32_t data_flags, size_t *len, const char **why) { void *ptr; + off_t probed; if (fd < 0) { *why = "the compositor handed us a DMA-BUF plane with no file descriptor"; @@ -708,21 +712,24 @@ static void *osc_map_dmabuf(int fd, size_t *len, const char **why) } /* * A DmaBuf plane can carry an advisory maxsize rather than its allocation - * length. The allocation size is a property of the exporting fd, and every - * dmabuf fd is seekable to that length. Probe unconditionally and prefer a - * larger real allocation, while retaining a meaningful producer bound when - * the fd cannot report one. + * length. The allocation size is a property of the exporting fd — dma_buf's + * llseek returns exactly dmabuf->size — so when the fd answers, it is the + * authority and maxsize is not. + * + * It has to win in BOTH directions. Preferring it only when it is LARGER + * leaves an over-declared maxsize as the mmap length, and the kernel refuses + * a dmabuf mapping longer than the object (-EINVAL), so the import dies + * reporting a driver that will not map — which is precisely the misdiagnosis + * this code exists to stop, with the right answer already in hand. */ - { - off_t probed = lseek(fd, 0, SEEK_END); - if (probed > 0 && (uintmax_t)probed <= SIZE_MAX && (size_t)probed > *len) { - size_t advertised = *len; - *len = (size_t)probed; - if (osc_debug_enabled()) { - fprintf(stderr, - "[osc-dmabuf] maxsize=%zu, recovered %zu bytes via lseek\n", - advertised, *len); - } + probed = lseek(fd, 0, SEEK_END); + if (probed > 0 && (uintmax_t)probed <= SIZE_MAX) { + size_t advertised = *len; + + *len = (size_t)probed; + if (advertised != *len && osc_debug_enabled()) { + fprintf(stderr, "[osc-dmabuf] maxsize=%zu, fd reports %zu bytes via lseek\n", + advertised, *len); } } if (*len == 0) { @@ -742,7 +749,14 @@ static void *osc_map_dmabuf(int fd, size_t *len, const char **why) } } if (ptr == MAP_FAILED) { - *why = "this driver does not allow CPU mapping of the capture buffer"; + /* The producer already answered this question. SPA_DATA_FLAG_MAPPABLE + * exists because "some memory types are not simply mappable (DmaBuf) + * unless explicitly specified with this flag" — so when it is unset, + * blaming the GPU driver sends the reader somewhere else entirely. */ + *why = SPA_FLAG_IS_SET(data_flags, SPA_DATA_FLAG_MAPPABLE) + ? "this driver does not allow CPU mapping of the capture buffer" + : "the compositor did not mark this DMA-BUF plane mappable " + "(SPA_DATA_FLAG_MAPPABLE unset)"; } return ptr == MAP_FAILED ? NULL : ptr; } @@ -761,6 +775,8 @@ static struct osc_dmabuf_map *osc_find_dmabuf_map(struct osc_pw_session *session enum osc_frame_bounds_error { OSC_FRAME_BOUNDS_OK, + /* Not a failure: a buffer carrying a cursor update and no pixels. */ + OSC_FRAME_BOUNDS_EMPTY_CHUNK, OSC_FRAME_BOUNDS_NO_CAPACITY, OSC_FRAME_BOUNDS_OFFSET, OSC_FRAME_BOUNDS_CORRUPTED, @@ -785,6 +801,20 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( * from the fd when the producer leaves a placeholder in maxsize. */ available = data_type == SPA_DATA_DmaBuf ? mapped_len : (size_t)maxsize; *available_out = available; + /* + * A zero-sized chunk is how a compositor ships a cursor update with no new + * frame attached; KWin is documented as doing exactly that. Not an error, + * just not a frame, and the caller returns without reporting a drop. + * + * It is asked HERE, ahead of everything else, so that this function and the + * production reader answer the same question. The check used to sit in + * osc_read_frame alone, which left the exported bound check accepting a + * buffer the reader silently refused — a contract the tests certified and + * the code did not implement. + */ + if (chunk_size == 0) { + return OSC_FRAME_BOUNDS_EMPTY_CHUNK; + } if (available == 0) { return OSC_FRAME_BOUNDS_NO_CAPACITY; } @@ -792,20 +822,9 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( if (offset > available) { return OSC_FRAME_BOUNDS_OFFSET; } - if ((chunk_flags & SPA_CHUNK_FLAG_CORRUPTED) != 0) { + if (SPA_FLAG_IS_SET(chunk_flags, SPA_CHUNK_FLAG_CORRUPTED)) { return OSC_FRAME_BOUNDS_CORRUPTED; } - if (data_type == SPA_DATA_DmaBuf) { - /* DMA-BUF capacity belongs to the fd, not to SPA's advisory maxsize or - * chunk size. Backends use different positive placeholders for those - * fields, so keying this path on magic values is both brittle and - * unnecessary. The frame is still accepted only when stride * height - * fits inside the actual mapped allocation below. */ - size = available - offset; - } else { - /* Validate offset first so this subtraction cannot underflow. */ - size = SPA_MIN((size_t)chunk_size, available - offset); - } if (stride <= 0 || width <= 0 || height <= 0) { return OSC_FRAME_BOUNDS_GEOMETRY; } @@ -815,6 +834,28 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( } /* Widen before multiplying: both operands originate outside this process. */ frame_bytes = (uint64_t)stride * (uint64_t)height; + /* + * `chunk->size` is the producer's statement of how many bytes it actually + * wrote, and clamping to it is what stops a short or torn frame from being + * read as a whole one. Some DMA-BUF backends leave a placeholder there + * instead — xdg-desktop-portal-wlr 0.8.2 writes 9, niri writes 1 — and + * clamping to those rejected every frame on those compositors, which is the + * bug this path exists to fix. + * + * So weigh the VALUE rather than the memory type: a chunk size too small to + * hold even one row of pixels is not a byte count, and the mapped allocation + * is then the only bound left. Anything at or above a row is taken at its + * word. One row is the threshold on purpose — the placeholders seen in the + * wild are a couple of bytes, three orders of magnitude below it, while a + * half-written frame is far above and stays clamped, so a torn frame is + * still refused instead of being read as a whole one. + */ + if (data_type == SPA_DATA_DmaBuf && (uint64_t)chunk_size < row_bytes) { + size = available - offset; + } else { + /* Offset was validated above, so this subtraction cannot underflow. */ + size = SPA_MIN((size_t)chunk_size, available - offset); + } if (frame_bytes > (uint64_t)size) { return OSC_FRAME_BOUNDS_FRAME_TOO_LARGE; } @@ -823,22 +864,12 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( return OSC_FRAME_BOUNDS_OK; } -int osc_pw_frame_bounds_valid(uint32_t data_type, uint32_t maxsize, size_t mapped_len, - uint32_t chunk_offset, uint32_t chunk_size, int32_t chunk_flags, - int32_t stride, int32_t width, int32_t height) -{ - size_t available; - size_t offset; - size_t size; - - return osc_resolve_frame_bounds(data_type, maxsize, mapped_len, chunk_offset, chunk_size, - chunk_flags, stride, width, height, &available, &offset, &size) == - OSC_FRAME_BOUNDS_OK; -} static const char *osc_frame_bounds_error_name(enum osc_frame_bounds_error error) { switch (error) { + case OSC_FRAME_BOUNDS_EMPTY_CHUNK: + return "chunk-size-zero"; case OSC_FRAME_BOUNDS_NO_CAPACITY: return "buffer-length-zero"; case OSC_FRAME_BOUNDS_OFFSET: @@ -857,6 +888,20 @@ static const char *osc_frame_bounds_error_name(enum osc_frame_bounds_error error return "unknown"; } +const char *osc_pw_frame_bounds_reason(uint32_t data_type, uint32_t maxsize, size_t mapped_len, + uint32_t chunk_offset, uint32_t chunk_size, + int32_t chunk_flags, int32_t stride, int32_t width, + int32_t height) +{ + size_t available = 0; + size_t offset = 0; + size_t size = 0; + + return osc_frame_bounds_error_name(osc_resolve_frame_bounds( + data_type, maxsize, mapped_len, chunk_offset, chunk_size, chunk_flags, stride, width, + height, &available, &offset, &size)); +} + static const char *osc_data_type_name(uint32_t data_type) { switch (data_type) { @@ -871,22 +916,54 @@ static const char *osc_data_type_name(uint32_t data_type) } } +/* + * The chunk fields arrive as values, not as a `chunk` pointer to re-read. This + * only ever runs when the producer has already misbehaved, which is exactly the + * moment it may be rewriting that struct; reading it a second time here would + * let the line describe a frame other than the one that was refused. + */ static void osc_report_frame_drop(struct osc_pw_session *session, const struct spa_data *data, - size_t mapped_len, size_t available, + size_t mapped_len, size_t available, uint32_t chunk_offset, + uint32_t chunk_size, int32_t chunk_flags, int32_t stride, enum osc_frame_bounds_error error) { - if (!osc_debug_enabled() || session->frame_drop_reports >= OSC_FRAME_DROP_REPORTS) { + const char *reason = osc_frame_bounds_error_name(error); + + if (session->frame_drop_reports >= OSC_FRAME_DROP_REPORTS) { return; } session->frame_drop_reports++; - fprintf(stderr, - "[osc-frame-drop] reason=%s data_type=%s data_type_id=%u fd=%lld maxsize=%u " - "mapped_len=%zu available_len=%zu chunk_offset=%u chunk_size=%u " - "chunk_flags=%d stride=%d width=%u height=%u format=%u\n", - osc_frame_bounds_error_name(error), osc_data_type_name(data->type), data->type, - (long long)data->fd, data->maxsize, mapped_len, available, data->chunk->offset, - data->chunk->size, data->chunk->flags, data->chunk->stride, session->format.size.width, - session->format.size.height, session->format.format); + + /* + * On the event stream, not only in a debug log. Every reason this reports is + * decided by stride, geometry and the mapped length, all of which are fixed + * for a whole negotiation — so a buffer that fails once fails every time and + * the user gets a recording with no frames in it. Nothing downstream can + * name the cause: a refused frame never reaches the mailbox, so the + * frames-dropped counter stays at zero and the session still stops + * "successfully". Requiring OPENSCREEN_PIPEWIRE_DEBUG and a reproduction to + * learn why is the diagnosis gap this whole change set exists to close. + */ + if (session->callbacks.on_capture_issue != NULL) { + char detail[320]; + + snprintf(detail, sizeof(detail), + "the compositor's buffer failed validation (%s), so this frame was dropped: " + "dataType=%s stride=%d width=%u height=%u chunkOffset=%u chunkSize=%u " + "available=%zu", + reason, osc_data_type_name(data->type), stride, session->format.size.width, + session->format.size.height, chunk_offset, chunk_size, available); + session->callbacks.on_capture_issue(session->callbacks.user, "frame-dropped", detail); + } + if (osc_debug_enabled()) { + fprintf(stderr, + "[osc-frame-drop] reason=%s data_type=%s data_type_id=%u fd=%lld maxsize=%u " + "mapped_len=%zu available_len=%zu chunk_offset=%u chunk_size=%u " + "chunk_flags=%d stride=%d width=%u height=%u format=%u\n", + reason, osc_data_type_name(data->type), data->type, (long long)data->fd, + data->maxsize, mapped_len, available, chunk_offset, chunk_size, chunk_flags, stride, + session->format.size.width, session->format.size.height, session->format.format); + } } /* @@ -912,6 +989,17 @@ static void osc_dmabuf_sync(int fd, int start) } } +static void osc_report_capture_issue(struct osc_pw_session *session, const char *code, + const char *detail) +{ + if (session->callbacks.on_capture_issue == NULL || + session->capture_issue_reports >= OSC_BUFFER_INFO_REPORTS) { + return; + } + session->capture_issue_reports++; + session->callbacks.on_capture_issue(session->callbacks.user, code, detail); +} + static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) { struct osc_pw_session *session = userdata; @@ -927,45 +1015,56 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) if (data->type != SPA_DATA_DmaBuf) { return; } + /* + * Several pw_buffers can be slices of ONE exported allocation and therefore + * arrive carrying the same fd. The table is keyed on that fd and the lookup + * returns the first match, so mapping it again buys a second mapping of the + * whole allocation that nothing will ever read, plus an unmap that only ever + * reaches one of the two. + */ + if (osc_find_dmabuf_map(session, (int)data->fd) != NULL) { + return; + } for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { if (session->dmabuf_maps[i].ptr != NULL) { continue; } - /* `maxsize` is the producer's statement of how much of the fd belongs to - * this buffer, and mapping exactly that keeps the bounds checks in - * osc_read_frame meaningful. It is legitimately 0 for a DMA-BUF plane — - * wlroots leaves it unset — in which case osc_map_dmabuf recovers the - * real length from the fd and reports it back here. Storing the - * producer's 0 instead would leave every later bounds check comparing - * against an empty mapping. */ + /* `maxsize` is a starting point, not the answer: osc_map_dmabuf prefers + * the length the fd itself reports, because a DmaBuf plane can carry an + * advisory maxsize — wlroots leaves it at 0 — rather than its allocation + * length. It reports back the length actually mapped, and THAT is what + * every later bounds check measures against. */ maplen = data->maxsize; - session->dmabuf_maps[i].ptr = osc_map_dmabuf((int)data->fd, &maplen, &why); + session->dmabuf_maps[i].ptr = osc_map_dmabuf((int)data->fd, data->flags, &maplen, &why); if (session->dmabuf_maps[i].ptr == NULL) { - /* Reported once, through the buffer-info channel that already exists - * for describing what the compositor handed us — a mapping failure - * here means no frames at all, and silence would read as a hang. + /* Reported once, on the event stream: a mapping failure here means + * no frames at all, and silence would read as a hang. * - * The reason is carried up rather than assumed: this used to say the + * The reason is carried up rather than assumed. This used to say the * driver refused CPU mapping no matter what actually went wrong, and * that message sent the one real investigation of this path looking * at the GPU for a size the compositor had simply left at 0. */ - if (session->callbacks.on_buffer_info != NULL && - session->buffer_info_reports < OSC_BUFFER_INFO_REPORTS) { - char detail[256]; - - snprintf(detail, sizeof(detail), "dmabuf import failed: %s; capture cannot proceed", - why); - session->buffer_info_reports++; - session->callbacks.on_buffer_info(session->callbacks.user, data->type, - pw_buf->buffer->n_datas, 0, 0, detail); - } + char detail[256]; + + snprintf(detail, sizeof(detail), "dmabuf import failed: %s; capture cannot proceed", + why); + osc_report_capture_issue(session, "dmabuf-import-failed", detail); return; } session->dmabuf_maps[i].fd = (int)data->fd; session->dmabuf_maps[i].len = maplen; return; } + /* + * Out of slots. Falling out of this loop used to be the one failure here + * that said nothing on any channel: osc_read_frame's lookup then misses for + * every frame of the session and returns before it can report anything, so + * the recording freezes with no explanation anywhere. + */ + osc_report_capture_issue(session, "dmabuf-map-table-full", + "the compositor allocated more DMA-BUF buffers than this helper can " + "map; capture cannot proceed"); } static void osc_on_remove_buffer(void *userdata, struct pw_buffer *pw_buf) @@ -1106,12 +1205,16 @@ static int osc_read_cursor(const struct spa_buffer *buffer, struct osc_pw_cursor * Extracts the pixels of one buffer. Returns 1 when `out` describes a frame, 0 * when this buffer carries none. * - * The offset/size clamping against the mapped length is the standard PipeWire - * consumer idiom and is not paranoia: `chunk` lives in memory the PRODUCER - * writes, so its fields are untrusted input from another process. A compositor - * bug — or a malicious one — that reports a size past the end of the mapping - * would otherwise be a read straight off the end of the shared memory. For - * DMA-BUF, that length comes from dmabuf_maps[] because maxsize may be zero. + * Every bound comes out of osc_resolve_frame_bounds, and none of it is + * paranoia: `chunk` lives in memory the PRODUCER writes, so its fields are + * untrusted input from another process. A compositor bug — or a malicious one — + * that reports a size past the end of the mapping would otherwise be a read + * straight off the end of the shared memory. + * + * The readable length is the MAPPING's length. For MemPtr/MemFd pw_stream maps + * the region and `maxsize` is that length; for DMA-BUF we map it ourselves and + * dmabuf_maps[] records what was actually mapped, because SPA's maxsize is + * advisory there and some producers leave a placeholder in it. */ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffer *buffer, struct osc_pw_frame *out) @@ -1121,11 +1224,18 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe struct spa_meta_header *header; struct spa_meta_region *region; enum osc_frame_bounds_error bounds_error; - size_t available; + size_t available = 0; size_t mapped_len; - size_t offset; - size_t size; + /* Written by osc_resolve_frame_bounds only when it returns OK. Initialised + * so that reading them on any other path is a zero, never stack junk that + * would reach SPA_PTROFF as a pointer offset. */ + size_t offset = 0; + size_t size = 0; + uint32_t chunk_offset; + uint32_t chunk_size; + int32_t chunk_flags; int32_t stride; + int32_t width; int32_t height; const uint8_t *base; @@ -1164,20 +1274,27 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe base = data->data; mapped_len = data->maxsize; } - /* A zero-sized chunk is how a compositor ships a cursor update with no new - * frame attached. Not an error, just not a frame. */ - if (data->chunk->size == 0) { - return 0; - } - + width = (int32_t)session->format.size.width; height = (int32_t)session->format.size.height; + /* Read each producer-written field ONCE, and decide and report from that + * same snapshot. Re-reading them for the diagnostic would let a misbehaving + * producer rewrite the numbers in between, so the report would describe a + * frame other than the one that was refused. */ + chunk_offset = data->chunk->offset; + chunk_size = data->chunk->size; + chunk_flags = data->chunk->flags; stride = data->chunk->stride; - bounds_error = osc_resolve_frame_bounds( - data->type, data->maxsize, mapped_len, data->chunk->offset, data->chunk->size, - data->chunk->flags, stride, (int32_t)session->format.size.width, height, &available, &offset, - &size); + bounds_error = + osc_resolve_frame_bounds(data->type, data->maxsize, mapped_len, chunk_offset, chunk_size, + chunk_flags, stride, width, height, &available, &offset, &size); + /* A cursor update with no pixels attached. Expected traffic, not a drop, so + * it is deliberately not reported. */ + if (bounds_error == OSC_FRAME_BOUNDS_EMPTY_CHUNK) { + return 0; + } if (bounds_error != OSC_FRAME_BOUNDS_OK) { - osc_report_frame_drop(session, data, mapped_len, available, bounds_error); + osc_report_frame_drop(session, data, mapped_len, available, chunk_offset, chunk_size, + chunk_flags, stride, bounds_error); return 0; } @@ -1194,7 +1311,7 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe out->data = SPA_PTROFF(base, offset, const uint8_t); out->size = size; out->stride = stride; - out->width = (int32_t)session->format.size.width; + out->width = width; out->height = height; out->video_format = session->format.format; diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index 70a90ab14..56325cc86 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -122,6 +122,18 @@ struct osc_pw_callbacks { void (*on_buffer_info)(void *user, uint32_t data_type, uint32_t n_datas, int32_t has_cursor_meta, uint32_t cursor_meta_size, const char *metas); + /* A condition that costs the user frames, reported on the event stream + * rather than to a debug log. + * + * This channel exists because the failures it carries are TERMINAL and + * SILENT: stride, geometry and the mapped length are constant for a whole + * negotiation, so a buffer that fails validation once fails every time, and + * the user gets a recording with no frames in it. Nothing downstream can + * infer the cause — a rejected frame never reaches the mailbox, so the + * frames-dropped counter stays at zero and the session still stops + * "successfully". `code` is a stable kebab-case identifier; `detail` is a + * borrowed sentence for a human. */ + void (*on_capture_issue)(void *user, const char *code, const char *detail); void (*on_state)(void *user, const char *state, const char *error); }; @@ -171,10 +183,25 @@ int osc_pw_cursor_meta_accepts_producer_size(uint32_t width, uint32_t height); */ int osc_pw_enum_format_accepts_dmabuf_producer(int with_modifier, int64_t producer_modifier); -/* Test-only frame-bound validation without a live PipeWire buffer. */ -int osc_pw_frame_bounds_valid(uint32_t data_type, uint32_t maxsize, size_t mapped_len, - uint32_t chunk_offset, uint32_t chunk_size, int32_t chunk_flags, - int32_t stride, int32_t width, int32_t height); +/* + * Which bound, if any, rejects this frame? Returns the same stable reason string + * the frame-drop diagnostics carry ("stride-shorter-than-row", + * "chunk-offset-out-of-bounds", ...), or "none" when the frame is accepted. + * + * It returns the REASON rather than a yes/no because six distinct conditions + * reject a frame here, and a test asserting only "rejected" passes when the + * wrong one fires — a reordering of the checks, or a broken row computation, + * would keep such a suite green. Exposed so the bound can be asserted without a + * portal, a compositor, or a screen. + * + * The caller's pre-checks in osc_read_frame (n_datas, a NULL chunk, a missing + * dmabuf mapping) are deliberately NOT modelled here: this answers only the + * bounds question. + */ +const char *osc_pw_frame_bounds_reason(uint32_t data_type, uint32_t maxsize, size_t mapped_len, + uint32_t chunk_offset, uint32_t chunk_size, + int32_t chunk_flags, int32_t stride, int32_t width, + int32_t height); struct osc_pw_session; diff --git a/electron/native/pipewire-capture/src/main.rs b/electron/native/pipewire-capture/src/main.rs index b52e23b12..92dd534e5 100644 --- a/electron/native/pipewire-capture/src/main.rs +++ b/electron/native/pipewire-capture/src/main.rs @@ -953,6 +953,19 @@ fn run( } } + // Surfaced as a warning rather than logged, because these are the + // conditions that leave a recording empty while every counter still + // reads zero: a frame refused by the shim never reaches the mailbox, + // so `frames-dropped` stays at 0 and the session stops "cleanly". + // Without this the only honest symptom is a file with no frames in + // it and nothing anywhere saying why. + Ok(Message::Stream(StreamEvent::CaptureIssue { code, detail })) => { + let _ = emitter.emit(&Event::Warning { + code, + message: detail, + }); + } + Ok(Message::Stream(StreamEvent::State { state, error })) => { // Every transition, not just the failures. Without this a run // that ends in "target not found" is ambiguous: there is no way diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index 28df2d69c..caddbf132 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -65,6 +65,7 @@ struct RawCallbacks { on_cursor: extern "C" fn(*mut c_void, *const RawCursor), on_frame: extern "C" fn(*mut c_void, *const RawFrame), on_buffer_info: extern "C" fn(*mut c_void, u32, u32, i32, u32, *const c_char), + on_capture_issue: extern "C" fn(*mut c_void, *const c_char, *const c_char), on_state: extern "C" fn(*mut c_void, *const c_char, *const c_char), } @@ -103,7 +104,7 @@ extern "C" { producer_modifier: i64, ) -> i32; #[cfg(test)] - fn osc_pw_frame_bounds_valid( + fn osc_pw_frame_bounds_reason( data_type: u32, maxsize: u32, mapped_len: usize, @@ -113,7 +114,7 @@ extern "C" { stride: i32, width: i32, height: i32, - ) -> i32; + ) -> *const c_char; fn osc_pw_start( fd: i32, node_id: u32, @@ -143,6 +144,15 @@ pub enum StreamEvent { /// negotiation. Empty when the buffers carry none at all. metas: String, }, + /// Something cost the user frames and nothing downstream could work out + /// what. Raised by the shim for conditions that are terminal and otherwise + /// silent — a DMA-BUF import that failed, a buffer that fails validation — + /// where the recording comes out empty and every counter still reads zero. + CaptureIssue { + /// Stable kebab-case identifier, surfaced as the warning's `code`. + code: String, + detail: String, + }, Cursor(CursorEvent), /// A frame is waiting in the [`FrameMailbox`]. Carries no payload on /// purpose: an 8 MB frame per channel message would allocate and copy far @@ -731,8 +741,15 @@ pub fn enum_format_accepts_dmabuf_producer(with_modifier: bool, producer_modifie unsafe { osc_pw_enum_format_accepts_dmabuf_producer(i32::from(with_modifier), producer_modifier) } } +/// The inputs `osc_resolve_frame_bounds` weighs, as named fields. +/// +/// The C entry point takes nine bare integers, four of which are plausible +/// neighbours (`maxsize`/`mapped_len`, `chunk_offset`/`chunk_size`) that a +/// transposition would not disturb. Building from a healthy baseline and +/// changing one field means each test says which input it is about. #[cfg(test)] -fn frame_bounds_valid( +#[derive(Clone, Copy)] +struct Bounds { data_type: u32, maxsize: u32, mapped_len: usize, @@ -742,20 +759,63 @@ fn frame_bounds_valid( stride: i32, width: i32, height: i32, -) -> bool { - // SAFETY: the C helper performs arithmetic only and owns its output storage. - unsafe { - osc_pw_frame_bounds_valid( - data_type, - maxsize, - mapped_len, - chunk_offset, - chunk_size, - chunk_flags, - stride, - width, - height, - ) != 0 +} + +#[cfg(test)] +impl Bounds { + /// A healthy 1920x1080 BGRx frame in the 8 MiB allocation a GPU hands back + /// for it — the shape a working DMA-BUF session delivers, with an honest + /// `chunk_size`. + fn dmabuf_1080p() -> Self { + let stride = 1920 * 4; + Self { + data_type: constants().data_dma_buf, + maxsize: 0, + mapped_len: 8 * 1024 * 1024, + chunk_offset: 0, + chunk_size: stride * 1080, + chunk_flags: 0, + stride: stride as i32, + width: 1920, + height: 1080, + } + } + + /// The same frame over shared memory, where `maxsize` really is the mapping + /// length because pw_stream mapped it. + fn memfd_1080p() -> Self { + let stride = 1920u32 * 4; + Self { + data_type: constants().data_mem_fd, + maxsize: stride * 1080, + mapped_len: (stride * 1080) as usize, + ..Self::dmabuf_1080p() + } + } + + /// Which bound rejects this frame, or `"none"` when it is accepted. + fn reason(self) -> String { + // SAFETY: the C helper performs arithmetic only and returns a pointer to + // one of its own string literals, which outlives this call. + let raw = unsafe { + osc_pw_frame_bounds_reason( + self.data_type, + self.maxsize, + self.mapped_len, + self.chunk_offset, + self.chunk_size, + self.chunk_flags, + self.stride, + self.width, + self.height, + ) + }; + assert!(!raw.is_null(), "the shim always names a reason"); + unsafe { CStr::from_ptr(raw) }.to_string_lossy().into_owned() + } + + fn is_accepted(self) -> bool { + self.reason() == "none" } } @@ -805,6 +865,7 @@ impl Session { on_cursor, on_frame, on_buffer_info, + on_capture_issue, on_state, }; @@ -969,6 +1030,24 @@ extern "C" fn on_cursor(user: *mut c_void, cursor: *const RawCursor) { }); } +extern "C" fn on_capture_issue(user: *mut c_void, code: *const c_char, detail: *const c_char) { + with_sink(user, |sink| { + let owned = |raw: *const c_char| { + if raw.is_null() { + String::new() + } else { + // SAFETY: the shim passes NUL-terminated buffers that outlive the + // callback. + unsafe { CStr::from_ptr(raw) }.to_string_lossy().into_owned() + } + }; + sink(StreamEvent::CaptureIssue { + code: owned(code), + detail: owned(detail), + }); + }); +} + extern "C" fn on_state(user: *mut c_void, state: *const c_char, error: *const c_char) { with_sink(user, |sink| { let to_string = |raw: *const c_char| { @@ -1078,166 +1157,165 @@ mod tests { } #[test] - fn dmabuf_uses_the_mapped_allocation_not_advisory_sizes() { - let constants = constants(); - let stride = 1920 * 4; + fn a_healthy_dmabuf_frame_is_accepted() { + assert_eq!(Bounds::dmabuf_1080p().reason(), "none"); + } - // Portal backends use different placeholders for DMA-BUF sizes. None - // are special: the fd allocation and frame geometry are authoritative. - assert!(frame_bounds_valid( - constants.data_dma_buf, - 0, - 8 * 1024 * 1024, - 0, - 9, - 0, - stride as i32, - 1920, - 1080, - )); - assert!(frame_bounds_valid( - constants.data_dma_buf, - 1, - 8 * 1024 * 1024, - 0, - 1, - 0, - stride as i32, - 1920, - 1080, - )); - assert!(frame_bounds_valid( - constants.data_dma_buf, - 37, - 8 * 1024 * 1024, - 0, - 23, - 0, - stride as i32, - 1920, - 1080, - )); - assert!(!frame_bounds_valid( - constants.data_dma_buf, - u32::MAX, - 4096, - 0, - u32::MAX, - 0, - stride as i32, - 1920, - 1080, - )); + /// The fix for issue #287. Several portal backends put a token value in + /// `chunk->size` for a DMA-BUF plane instead of a byte count — xdg-desktop- + /// portal-wlr 0.8.2 writes 9, niri writes 1 — and clamping the frame to it + /// rejected every frame those compositors ever sent. + #[test] + fn dmabuf_placeholder_chunk_sizes_are_ignored_rather_than_obeyed() { + for placeholder in [1, 9, 23] { + let frame = Bounds { + chunk_size: placeholder, + ..Bounds::dmabuf_1080p() + }; + assert_eq!( + frame.reason(), + "none", + "a chunk size of {placeholder} is a placeholder, not a byte count" + ); + } } + /// The other half of that rule: a chunk size large enough to be a real byte + /// count is still believed. A compositor whose copy did not finish reports a + /// short chunk, and reading it as a whole frame splices the previous frame's + /// pixels into the bottom of this one. #[test] - fn frame_bounds_reject_invalid_offsets_and_geometry_without_affecting_memfd() { + fn a_torn_dmabuf_frame_is_still_refused() { + let healthy = Bounds::dmabuf_1080p(); + let half_written = Bounds { + chunk_size: healthy.chunk_size / 2, + ..healthy + }; + assert_eq!( + half_written.reason(), + "frame-bytes-exceed-available-chunk", + "half a frame is a plausible byte count and must still be clamped" + ); + } + + /// `maxsize` is advisory on the DMA-BUF path, so it can be arbitrarily + /// large — and being large must not let a frame reach past what was mapped. + #[test] + fn advisory_sizes_cannot_stretch_a_frame_past_its_mapping() { + let frame = Bounds { + maxsize: u32::MAX, + mapped_len: 4096, + ..Bounds::dmabuf_1080p() + }; + assert_eq!(frame.reason(), "frame-bytes-exceed-available-chunk"); + } + + /// Every rejection, each reached by perturbing exactly one field of a frame + /// that is otherwise healthy, and each asserted by NAME. Asserting only + /// "rejected" would pass when the wrong bound fires — which is exactly what + /// a reordering of these checks would do. + #[test] + fn each_rejection_names_itself() { + let healthy = Bounds::dmabuf_1080p(); + let cases = [ + ("chunk-size-zero", Bounds { chunk_size: 0, ..healthy }), + ("buffer-length-zero", Bounds { mapped_len: 0, ..healthy }), + ( + "chunk-offset-out-of-bounds", + Bounds { + chunk_offset: healthy.mapped_len as u32 + 1, + ..healthy + }, + ), + ( + "producer-marked-frame-corrupted", + Bounds { + chunk_flags: 1, // SPA_CHUNK_FLAG_CORRUPTED + ..healthy + }, + ), + ( + "stride-shorter-than-row", + Bounds { + stride: healthy.width * 4 - 1, + ..healthy + }, + ), + ( + "frame-bytes-exceed-available-chunk", + Bounds { + mapped_len: 1024 * 1024, + ..healthy + }, + ), + ]; + for (expected, frame) in cases { + assert_eq!(frame.reason(), expected); + } + } + + /// The geometry gate moved during the bounds refactor and its `width` term + /// is new, so every axis is pinned here. Nothing else in the suite reaches + /// this branch: a healthy frame's stride, width and height are all positive. + #[test] + fn geometry_is_rejected_on_every_axis() { + let healthy = Bounds::dmabuf_1080p(); + let broken = [ + ("zero stride", Bounds { stride: 0, ..healthy }), + ("negative stride", Bounds { stride: -1, ..healthy }), + ("zero width", Bounds { width: 0, ..healthy }), + ("negative width", Bounds { width: -1, ..healthy }), + ("zero height", Bounds { height: 0, ..healthy }), + ("negative height", Bounds { height: -1, ..healthy }), + ]; + for (axis, frame) in broken { + assert_eq!(frame.reason(), "invalid-frame-geometry", "{axis}"); + } + } + + /// A buffer carrying a cursor update and no pixels. The reader returns + /// without reporting a drop, but the bound still has to NAME it, or the + /// exported check and the production reader disagree about what it is. + #[test] + fn a_cursor_only_buffer_is_not_a_frame() { + for base in [Bounds::dmabuf_1080p(), Bounds::memfd_1080p()] { + let cursor_only = Bounds { chunk_size: 0, ..base }; + assert_eq!(cursor_only.reason(), "chunk-size-zero"); + } + } + + /// The DMA-BUF leniency must not leak to shared memory, where `maxsize` is + /// the real mapping length and `chunk->size` is a real byte count. MemPtr + /// and MemFd are both advertised by the video path, so both are exercised. + #[test] + fn shared_memory_still_clamps_to_the_declared_chunk_size() { let constants = constants(); - let stride = 16; - let frame_len = stride * 4; + for data_type in [constants.data_mem_fd, constants.data_mem_ptr] { + let healthy = Bounds { + data_type, + ..Bounds::memfd_1080p() + }; + assert!(healthy.is_accepted(), "a healthy shared-memory frame"); + let placeholder = Bounds { chunk_size: 9, ..healthy }; + assert_eq!( + placeholder.reason(), + "frame-bytes-exceed-available-chunk", + "9 bytes is 9 bytes on shared memory, not a placeholder" + ); + } + } - assert!(frame_bounds_valid( - constants.data_dma_buf, - frame_len, - frame_len as usize, - 0, - frame_len, - 0, - stride as i32, - 4, - 4, - )); - assert!(!frame_bounds_valid( - constants.data_dma_buf, - frame_len, - frame_len as usize, - frame_len + 1, - frame_len, - 0, - stride as i32, - 4, - 4, - )); - // DMA-BUF advisory sizes cannot extend the actual fd allocation. - assert!(frame_bounds_valid( - constants.data_dma_buf, - frame_len, - frame_len as usize, - 0, - u32::MAX, - 0, - stride as i32, - 4, - 4, - )); - // Shared-memory buffers continue to use PipeWire's maxsize; a separate - // mapped length is meaningful only for DMA-BUF. - assert!(frame_bounds_valid( - constants.data_mem_fd, - frame_len, - 0, - 0, - frame_len, - 0, - stride as i32, - 4, - 4, - )); - assert!(!frame_bounds_valid( - constants.data_mem_fd, - frame_len, - 0, - 0, - frame_len, - 1, - stride as i32, - 4, - 4, - )); - assert!(!frame_bounds_valid( - constants.data_mem_fd, - 0, - frame_len as usize, - 0, - frame_len, - 0, - stride as i32, - 4, - 4, - )); - assert!(!frame_bounds_valid( - constants.data_dma_buf, - u32::MAX, - u32::MAX as usize, - 0, - u32::MAX, - 0, - i32::MAX, - 4, - i32::MAX, - )); - assert!(!frame_bounds_valid( - constants.data_dma_buf, - frame_len, - frame_len as usize, - 0, - frame_len, - 0, - 15, - 4, - 4, - )); - assert!(!frame_bounds_valid( - constants.data_dma_buf, - 0, - frame_len as usize, - 0, - 9, - 1, - stride as i32, - 4, - 4, - )); + /// stride * height is computed on widened operands because both come from + /// another process; multiplied as int32 these wrap to a small positive + /// number and the frame would be accepted. + #[test] + fn an_overflowing_frame_size_cannot_wrap_past_the_bound() { + let frame = Bounds { + stride: i32::MAX, + height: i32::MAX, + ..Bounds::dmabuf_1080p() + }; + assert_eq!(frame.reason(), "frame-bytes-exceed-available-chunk"); } /// End-to-end exercise of the PipeWire half with NO portal involved. @@ -1335,6 +1413,12 @@ mod tests { Ok(StreamEvent::Cursor(cursor)) => { println!("[{:>5}ms] cursor {cursor:?}", stamp(std::time::Instant::now())); } + Ok(StreamEvent::CaptureIssue { code, detail }) => { + println!( + "[{:>5}ms] capture-issue {code}: {detail}", + stamp(std::time::Instant::now()) + ); + } // Unreachable: this session was started with no mailbox, so the // C side was never asked for frames. Matched rather than // wildcarded so that adding a variant is a compile error here From d0bf94268d764555d5306565b5a20fada0d0e0e2 Mon Sep 17 00:00:00 2001 From: Mamdasan Sabrian Date: Sun, 23 Aug 2026 23:17:14 +0200 Subject: [PATCH 4/4] fix(linux): fix DMA-BUF sizing offsets and refs --- .../native/pipewire-capture/csrc/pw_shim.c | 172 +++++++++------ .../native/pipewire-capture/csrc/pw_shim.h | 4 + electron/native/pipewire-capture/src/shim.rs | 202 ++++++++++++------ 3 files changed, 245 insertions(+), 133 deletions(-) diff --git a/electron/native/pipewire-capture/csrc/pw_shim.c b/electron/native/pipewire-capture/csrc/pw_shim.c index 3c7452e39..8dcac71aa 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.c +++ b/electron/native/pipewire-capture/csrc/pw_shim.c @@ -73,7 +73,7 @@ static int osc_debug_enabled(void); #define OSC_DRM_FORMAT_MOD_INVALID 0x00ffffffffffffffULL /* - * Mapped dmabuf fds, keyed by fd. + * Mapped dmabuf planes, keyed by (fd, mapoffset). * * PipeWire reuses a small, fixed buffer set for the life of a negotiation, so * the mapping is established once per buffer in add_buffer and torn down in @@ -91,8 +91,10 @@ static int osc_debug_enabled(void); struct osc_dmabuf_map { int fd; + uint32_t mapoffset; void *ptr; size_t len; + size_t refs; }; /* @@ -696,12 +698,17 @@ static int osc_debug_enabled(void) return cached; } -/* - * `why` receives a caller-reportable reason on failure. The three ways this can - * fail are not interchangeable, and conflating them sent the one real - * investigation of this path looking at the GPU driver for an hour. - */ -static void *osc_map_dmabuf(int fd, uint32_t data_flags, size_t *len, const char **why) +int osc_pw_dmabuf_mapped_len(size_t allocation_len, uint32_t mapoffset, size_t *mapped_len) +{ + if (mapped_len == NULL || (size_t)mapoffset >= allocation_len) { + return 0; + } + *mapped_len = allocation_len - (size_t)mapoffset; + return 1; +} + +static void *osc_map_dmabuf(int fd, uint32_t data_flags, uint32_t mapoffset, size_t *len, + const char **why) { void *ptr; off_t probed; @@ -726,10 +733,15 @@ static void *osc_map_dmabuf(int fd, uint32_t data_flags, size_t *len, const char if (probed > 0 && (uintmax_t)probed <= SIZE_MAX) { size_t advertised = *len; - *len = (size_t)probed; + if (!osc_pw_dmabuf_mapped_len((size_t)probed, mapoffset, len)) { + *why = "the DMA-BUF plane's mapoffset is outside the fd allocation"; + return NULL; + } if (advertised != *len && osc_debug_enabled()) { - fprintf(stderr, "[osc-dmabuf] maxsize=%zu, fd reports %zu bytes via lseek\n", - advertised, *len); + fprintf(stderr, + "[osc-dmabuf] maxsize=%zu, fd reports %jd bytes, mapoffset=%u, " + "mapped_len=%zu\n", + advertised, (intmax_t)probed, mapoffset, *len); } } if (*len == 0) { @@ -739,13 +751,15 @@ static void *osc_map_dmabuf(int fd, uint32_t data_flags, size_t *len, const char *why = "the DMA-BUF plane reports no size and its fd is not seekable"; return NULL; } - ptr = mmap(NULL, *len, PROT_READ, MAP_SHARED, fd, 0); + ptr = mmap(NULL, *len, PROT_READ, MAP_SHARED, fd, (off_t)mapoffset); if (osc_debug_enabled()) { if (ptr == MAP_FAILED) { - fprintf(stderr, "[osc-dmabuf] mmap fd=%d len=%zu FAILED errno=%d (%s)\n", fd, *len, - errno, strerror(errno)); + fprintf(stderr, + "[osc-dmabuf] mmap fd=%d mapoffset=%u len=%zu FAILED errno=%d (%s)\n", fd, + mapoffset, *len, errno, strerror(errno)); } else { - fprintf(stderr, "[osc-dmabuf] mmap fd=%d len=%zu ok\n", fd, *len); + fprintf(stderr, "[osc-dmabuf] mmap fd=%d mapoffset=%u len=%zu ok\n", fd, mapoffset, + *len); } } if (ptr == MAP_FAILED) { @@ -761,22 +775,62 @@ static void *osc_map_dmabuf(int fd, uint32_t data_flags, size_t *len, const char return ptr == MAP_FAILED ? NULL : ptr; } -static struct osc_dmabuf_map *osc_find_dmabuf_map(struct osc_pw_session *session, int fd) +static struct osc_dmabuf_map *osc_find_dmabuf_map(struct osc_pw_session *session, int fd, + uint32_t mapoffset) { size_t i; for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { - if (session->dmabuf_maps[i].ptr != NULL && session->dmabuf_maps[i].fd == fd) { + if (session->dmabuf_maps[i].ptr != NULL && session->dmabuf_maps[i].fd == fd && + session->dmabuf_maps[i].mapoffset == mapoffset) { return &session->dmabuf_maps[i]; } } return NULL; } +static void osc_retain_dmabuf_map(struct osc_dmabuf_map *map) +{ + map->refs++; +} + +static int osc_release_dmabuf_map(struct osc_dmabuf_map *map) +{ + if (map->refs > 1) { + map->refs--; + return 0; + } + return 1; +} + +int osc_pw_dmabuf_map_lifecycle_valid(void) +{ + struct osc_pw_session session; + struct osc_dmabuf_map *map; + + memset(&session, 0, sizeof(session)); + map = &session.dmabuf_maps[0]; + map->fd = 7; + map->mapoffset = 4096; + map->ptr = (void *)(uintptr_t)1; + map->len = 8192; + map->refs = 1; + + if (osc_find_dmabuf_map(&session, 7, 4096) != map || + osc_find_dmabuf_map(&session, 7, 0) != NULL) { + return 0; + } + osc_retain_dmabuf_map(map); + if (map->refs != 2 || osc_release_dmabuf_map(map) != 0 || map->refs != 1) { + return 0; + } + return osc_release_dmabuf_map(map) == 1; +} + enum osc_frame_bounds_error { OSC_FRAME_BOUNDS_OK, - /* Not a failure: a buffer carrying a cursor update and no pixels. */ - OSC_FRAME_BOUNDS_EMPTY_CHUNK, + /* Not a failure: a buffer carrying metadata and no pixel rows. */ + OSC_FRAME_BOUNDS_METADATA_ONLY, OSC_FRAME_BOUNDS_NO_CAPACITY, OSC_FRAME_BOUNDS_OFFSET, OSC_FRAME_BOUNDS_CORRUPTED, @@ -796,24 +850,11 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( uint64_t row_bytes; uint64_t frame_bytes; - /* PipeWire maps MemPtr/MemFd for us and maxsize is their allocation bound. - * DMA-BUF is mapped by osc_on_add_buffer, which recovers the real length - * from the fd when the producer leaves a placeholder in maxsize. */ available = data_type == SPA_DATA_DmaBuf ? mapped_len : (size_t)maxsize; *available_out = available; - /* - * A zero-sized chunk is how a compositor ships a cursor update with no new - * frame attached; KWin is documented as doing exactly that. Not an error, - * just not a frame, and the caller returns without reporting a drop. - * - * It is asked HERE, ahead of everything else, so that this function and the - * production reader answer the same question. The check used to sit in - * osc_read_frame alone, which left the exported bound check accepting a - * buffer the reader silently refused — a contract the tests certified and - * the code did not implement. - */ - if (chunk_size == 0) { - return OSC_FRAME_BOUNDS_EMPTY_CHUNK; + /* Unlike advisory chunk_size, stride distinguishes metadata-only buffers. */ + if (stride == 0) { + return OSC_FRAME_BOUNDS_METADATA_ONLY; } if (available == 0) { return OSC_FRAME_BOUNDS_NO_CAPACITY; @@ -825,7 +866,7 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( if (SPA_FLAG_IS_SET(chunk_flags, SPA_CHUNK_FLAG_CORRUPTED)) { return OSC_FRAME_BOUNDS_CORRUPTED; } - if (stride <= 0 || width <= 0 || height <= 0) { + if (stride < 0 || width <= 0 || height <= 0) { return OSC_FRAME_BOUNDS_GEOMETRY; } row_bytes = (uint64_t)width * OSC_VIDEO_BYTES_PER_PIXEL; @@ -834,23 +875,8 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( } /* Widen before multiplying: both operands originate outside this process. */ frame_bytes = (uint64_t)stride * (uint64_t)height; - /* - * `chunk->size` is the producer's statement of how many bytes it actually - * wrote, and clamping to it is what stops a short or torn frame from being - * read as a whole one. Some DMA-BUF backends leave a placeholder there - * instead — xdg-desktop-portal-wlr 0.8.2 writes 9, niri writes 1 — and - * clamping to those rejected every frame on those compositors, which is the - * bug this path exists to fix. - * - * So weigh the VALUE rather than the memory type: a chunk size too small to - * hold even one row of pixels is not a byte count, and the mapped allocation - * is then the only bound left. Anything at or above a row is taken at its - * word. One row is the threshold on purpose — the placeholders seen in the - * wild are a couple of bytes, three orders of magnitude below it, while a - * half-written frame is far above and stays clamped, so a torn frame is - * still refused instead of being read as a whole one. - */ - if (data_type == SPA_DATA_DmaBuf && (uint64_t)chunk_size < row_bytes) { + /* DMA-BUF chunk_size is advisory; shared-memory chunk_size is a bound. */ + if (data_type == SPA_DATA_DmaBuf) { size = available - offset; } else { /* Offset was validated above, so this subtraction cannot underflow. */ @@ -864,12 +890,11 @@ static enum osc_frame_bounds_error osc_resolve_frame_bounds( return OSC_FRAME_BOUNDS_OK; } - static const char *osc_frame_bounds_error_name(enum osc_frame_bounds_error error) { switch (error) { - case OSC_FRAME_BOUNDS_EMPTY_CHUNK: - return "chunk-size-zero"; + case OSC_FRAME_BOUNDS_METADATA_ONLY: + return "metadata-only"; case OSC_FRAME_BOUNDS_NO_CAPACITY: return "buffer-length-zero"; case OSC_FRAME_BOUNDS_OFFSET: @@ -1004,6 +1029,7 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) { struct osc_pw_session *session = userdata; const char *why = "unknown reason"; + struct osc_dmabuf_map *existing; struct spa_data *data; size_t maplen; size_t i; @@ -1015,14 +1041,10 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) if (data->type != SPA_DATA_DmaBuf) { return; } - /* - * Several pw_buffers can be slices of ONE exported allocation and therefore - * arrive carrying the same fd. The table is keyed on that fd and the lookup - * returns the first match, so mapping it again buys a second mapping of the - * whole allocation that nothing will ever read, plus an unmap that only ever - * reaches one of the two. - */ - if (osc_find_dmabuf_map(session, (int)data->fd) != NULL) { + /* References keep a shared plane mapped until its last buffer is removed. */ + existing = osc_find_dmabuf_map(session, (int)data->fd, data->mapoffset); + if (existing != NULL) { + osc_retain_dmabuf_map(existing); return; } @@ -1036,7 +1058,8 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) * length. It reports back the length actually mapped, and THAT is what * every later bounds check measures against. */ maplen = data->maxsize; - session->dmabuf_maps[i].ptr = osc_map_dmabuf((int)data->fd, data->flags, &maplen, &why); + session->dmabuf_maps[i].ptr = + osc_map_dmabuf((int)data->fd, data->flags, data->mapoffset, &maplen, &why); if (session->dmabuf_maps[i].ptr == NULL) { /* Reported once, on the event stream: a mapping failure here means * no frames at all, and silence would read as a hang. @@ -1053,7 +1076,9 @@ static void osc_on_add_buffer(void *userdata, struct pw_buffer *pw_buf) return; } session->dmabuf_maps[i].fd = (int)data->fd; + session->dmabuf_maps[i].mapoffset = data->mapoffset; session->dmabuf_maps[i].len = maplen; + session->dmabuf_maps[i].refs = 1; return; } /* @@ -1077,15 +1102,24 @@ static void osc_on_remove_buffer(void *userdata, struct pw_buffer *pw_buf) return; } data = &pw_buf->buffer->datas[0]; + if (data->type != SPA_DATA_DmaBuf) { + return; + } for (i = 0; i < OSC_MAX_DMABUF_MAPS; i++) { if (session->dmabuf_maps[i].ptr == NULL || - session->dmabuf_maps[i].fd != (int)data->fd) { + session->dmabuf_maps[i].fd != (int)data->fd || + session->dmabuf_maps[i].mapoffset != data->mapoffset) { continue; } + if (!osc_release_dmabuf_map(&session->dmabuf_maps[i])) { + return; + } munmap(session->dmabuf_maps[i].ptr, session->dmabuf_maps[i].len); session->dmabuf_maps[i].ptr = NULL; session->dmabuf_maps[i].fd = -1; + session->dmabuf_maps[i].mapoffset = 0; session->dmabuf_maps[i].len = 0; + session->dmabuf_maps[i].refs = 0; return; } } @@ -1101,7 +1135,9 @@ static void osc_unmap_all_dmabufs(struct osc_pw_session *session) munmap(session->dmabuf_maps[i].ptr, session->dmabuf_maps[i].len); session->dmabuf_maps[i].ptr = NULL; session->dmabuf_maps[i].fd = -1; + session->dmabuf_maps[i].mapoffset = 0; session->dmabuf_maps[i].len = 0; + session->dmabuf_maps[i].refs = 0; } } @@ -1258,7 +1294,7 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe * osc_on_add_buffer. A miss means the mmap failed there — reported at * that point — and there is nothing readable here. */ - dmabuf_map = osc_find_dmabuf_map(session, (int)data->fd); + dmabuf_map = osc_find_dmabuf_map(session, (int)data->fd, data->mapoffset); if (dmabuf_map == NULL) { return 0; } @@ -1287,9 +1323,7 @@ static int osc_read_frame(struct osc_pw_session *session, const struct spa_buffe bounds_error = osc_resolve_frame_bounds(data->type, data->maxsize, mapped_len, chunk_offset, chunk_size, chunk_flags, stride, width, height, &available, &offset, &size); - /* A cursor update with no pixels attached. Expected traffic, not a drop, so - * it is deliberately not reported. */ - if (bounds_error == OSC_FRAME_BOUNDS_EMPTY_CHUNK) { + if (bounds_error == OSC_FRAME_BOUNDS_METADATA_ONLY) { return 0; } if (bounds_error != OSC_FRAME_BOUNDS_OK) { diff --git a/electron/native/pipewire-capture/csrc/pw_shim.h b/electron/native/pipewire-capture/csrc/pw_shim.h index 56325cc86..d4c5ec0ee 100644 --- a/electron/native/pipewire-capture/csrc/pw_shim.h +++ b/electron/native/pipewire-capture/csrc/pw_shim.h @@ -203,6 +203,10 @@ const char *osc_pw_frame_bounds_reason(uint32_t data_type, uint32_t maxsize, siz int32_t chunk_flags, int32_t stride, int32_t width, int32_t height); +/* DMA-BUF mapoffset and shared-mapping lifecycle test helpers. */ +int osc_pw_dmabuf_mapped_len(size_t allocation_len, uint32_t mapoffset, size_t *mapped_len); +int osc_pw_dmabuf_map_lifecycle_valid(void); + struct osc_pw_session; /* diff --git a/electron/native/pipewire-capture/src/shim.rs b/electron/native/pipewire-capture/src/shim.rs index caddbf132..7404b861c 100644 --- a/electron/native/pipewire-capture/src/shim.rs +++ b/electron/native/pipewire-capture/src/shim.rs @@ -115,6 +115,14 @@ extern "C" { width: i32, height: i32, ) -> *const c_char; + #[cfg(test)] + fn osc_pw_dmabuf_mapped_len( + allocation_len: usize, + mapoffset: u32, + mapped_len: *mut usize, + ) -> i32; + #[cfg(test)] + fn osc_pw_dmabuf_map_lifecycle_valid() -> i32; fn osc_pw_start( fd: i32, node_id: u32, @@ -309,6 +317,11 @@ impl FrameMailbox { } } +/// How much silence the ring will stand in for before it stops trying. +/// +/// This bounds the allocation after a long stall; drop accounting remains exact. +const MAX_SILENCE_SECONDS: usize = 30; + /// Interleaved samples waiting to be encoded. /// /// UNLIKE THE VIDEO MAILBOX, THIS IS A QUEUE. A dropped video frame costs one @@ -333,27 +346,6 @@ impl FrameMailbox { /// until someone drains, which is also the only thing that can stop it growing — /// and it stops being exact at [`MAX_SILENCE_SECONDS`], which is where a bounded /// allocation starts to matter more than sync nobody can still use. -/// How much silence the ring will stand in for before it stops trying. -/// -/// The debt costs a counter while it is owed and only becomes memory when -/// someone drains — 384 KB per second of it, at 48 kHz stereo f32. A drain runs -/// on every tick of the main loop, heartbeat included (`Capture::advance` from -/// the `RecvTimeoutError::Timeout` arm in main.rs), so a debt worth seconds -/// means the loop itself has stopped. There is no bound on how long a stopped -/// loop stays stopped, and one that comes back materialises the whole stall in a -/// single allocation — which is also the one place the ring's own two-second cap -/// does not reach. There is a second, quieter way to get there: nothing drains -/// before the first video frame either, so the debt grows for as long as the -/// portal picker is up. That normally ends in `clear` rather than a drain, but -/// not if staging that first frame fails, and the stop path flushes the ring. -/// -/// Thirty seconds is far past any stall a recording survives, and past it the -/// take has a hole half a minute wide — the cap trades sync that is already lost -/// for an allocation that stays bounded. `dropped_samples` keeps counting the -/// whole loss regardless, so the `audio-dropped` warning still reports what -/// really happened rather than what could be paid back. -const MAX_SILENCE_SECONDS: usize = 30; - #[derive(Debug)] pub struct AudioRing { inner: std::sync::Mutex, @@ -819,6 +811,15 @@ impl Bounds { } } +#[cfg(test)] +fn dmabuf_mapped_len(allocation_len: usize, mapoffset: u32) -> Option { + let mut mapped_len = 0; + // SAFETY: `mapped_len` is a live `usize` destination. The helper only + // performs checked arithmetic and writes through this pointer on success. + let valid = unsafe { osc_pw_dmabuf_mapped_len(allocation_len, mapoffset, &mut mapped_len) }; + (valid != 0).then_some(mapped_len) +} + /// SPA enum values as compiled from the vendored headers. pub fn constants() -> Constants { let mut out = Constants::default(); @@ -1161,43 +1162,57 @@ mod tests { assert_eq!(Bounds::dmabuf_1080p().reason(), "none"); } - /// The fix for issue #287. Several portal backends put a token value in - /// `chunk->size` for a DMA-BUF plane instead of a byte count — xdg-desktop- - /// portal-wlr 0.8.2 writes 9, niri writes 1 — and clamping the frame to it - /// rejected every frame those compositors ever sent. #[test] - fn dmabuf_placeholder_chunk_sizes_are_ignored_rather_than_obeyed() { - for placeholder in [1, 9, 23] { + fn dmabuf_chunk_size_is_always_advisory() { + let healthy = Bounds::dmabuf_1080p(); + let row_bytes = healthy.width as u32 * 4; + for chunk_size in [ + 0, + 1, + 9, + row_bytes - 1, + row_bytes, + healthy.chunk_size / 2, + healthy.chunk_size, + u32::MAX, + ] { let frame = Bounds { - chunk_size: placeholder, - ..Bounds::dmabuf_1080p() + chunk_size, + ..healthy }; assert_eq!( frame.reason(), "none", - "a chunk size of {placeholder} is a placeholder, not a byte count" + "DMA-BUF chunk size {chunk_size} must not replace the fd allocation bound" ); } } - /// The other half of that rule: a chunk size large enough to be a real byte - /// count is still believed. A compositor whose copy did not finish reports a - /// short chunk, and reading it as a whole frame splices the previous frame's - /// pixels into the bottom of this one. #[test] - fn a_torn_dmabuf_frame_is_still_refused() { - let healthy = Bounds::dmabuf_1080p(); - let half_written = Bounds { - chunk_size: healthy.chunk_size / 2, - ..healthy - }; + fn dmabuf_mapping_length_accounts_for_mapoffset() { + let allocation_len = 8 * 1024 * 1024; + assert_eq!(dmabuf_mapped_len(allocation_len, 0), Some(allocation_len)); + assert_eq!( + dmabuf_mapped_len(allocation_len, 4096), + Some(allocation_len - 4096) + ); + assert_eq!( + dmabuf_mapped_len(allocation_len, allocation_len as u32), + None + ); assert_eq!( - half_written.reason(), - "frame-bytes-exceed-available-chunk", - "half a frame is a plausible byte count and must still be clamped" + dmabuf_mapped_len(allocation_len, allocation_len as u32 + 4096), + None ); } + #[test] + fn shared_dmabuf_mapping_is_reference_counted_by_exact_plane() { + // SAFETY: the helper owns its synthetic session and touches no external + // resources; it only exercises lookup and reference-count transitions. + assert_eq!(unsafe { osc_pw_dmabuf_map_lifecycle_valid() }, 1); + } + /// `maxsize` is advisory on the DMA-BUF path, so it can be arbitrarily /// large — and being large must not let a frame reach past what was mapped. #[test] @@ -1218,8 +1233,20 @@ mod tests { fn each_rejection_names_itself() { let healthy = Bounds::dmabuf_1080p(); let cases = [ - ("chunk-size-zero", Bounds { chunk_size: 0, ..healthy }), - ("buffer-length-zero", Bounds { mapped_len: 0, ..healthy }), + ( + "metadata-only", + Bounds { + stride: 0, + ..healthy + }, + ), + ( + "buffer-length-zero", + Bounds { + mapped_len: 0, + ..healthy + }, + ), ( "chunk-offset-out-of-bounds", Bounds { @@ -1250,7 +1277,7 @@ mod tests { ), ]; for (expected, frame) in cases { - assert_eq!(frame.reason(), expected); + assert_eq!(frame.reason(), expected, "the {expected} case"); } } @@ -1261,26 +1288,58 @@ mod tests { fn geometry_is_rejected_on_every_axis() { let healthy = Bounds::dmabuf_1080p(); let broken = [ - ("zero stride", Bounds { stride: 0, ..healthy }), - ("negative stride", Bounds { stride: -1, ..healthy }), - ("zero width", Bounds { width: 0, ..healthy }), - ("negative width", Bounds { width: -1, ..healthy }), - ("zero height", Bounds { height: 0, ..healthy }), - ("negative height", Bounds { height: -1, ..healthy }), + ( + "negative stride", + Bounds { + stride: -1, + ..healthy + }, + ), + ( + "zero width", + Bounds { + width: 0, + ..healthy + }, + ), + ( + "negative width", + Bounds { + width: -1, + ..healthy + }, + ), + ( + "zero height", + Bounds { + height: 0, + ..healthy + }, + ), + ( + "negative height", + Bounds { + height: -1, + ..healthy + }, + ), ]; for (axis, frame) in broken { assert_eq!(frame.reason(), "invalid-frame-geometry", "{axis}"); } } - /// A buffer carrying a cursor update and no pixels. The reader returns - /// without reporting a drop, but the bound still has to NAME it, or the - /// exported check and the production reader disagree about what it is. #[test] - fn a_cursor_only_buffer_is_not_a_frame() { + fn a_zero_stride_buffer_is_metadata_only() { for base in [Bounds::dmabuf_1080p(), Bounds::memfd_1080p()] { - let cursor_only = Bounds { chunk_size: 0, ..base }; - assert_eq!(cursor_only.reason(), "chunk-size-zero"); + for chunk_size in [0, 9, base.chunk_size] { + let metadata = Bounds { + stride: 0, + chunk_size, + ..base + }; + assert_eq!(metadata.reason(), "metadata-only"); + } } } @@ -1296,15 +1355,30 @@ mod tests { ..Bounds::memfd_1080p() }; assert!(healthy.is_accepted(), "a healthy shared-memory frame"); - let placeholder = Bounds { chunk_size: 9, ..healthy }; - assert_eq!( - placeholder.reason(), - "frame-bytes-exceed-available-chunk", - "9 bytes is 9 bytes on shared memory, not a placeholder" - ); + for chunk_size in [0, 1, 9, healthy.chunk_size / 2] { + let short = Bounds { + chunk_size, + ..healthy + }; + assert_eq!( + short.reason(), + "frame-bytes-exceed-available-chunk", + "{chunk_size} bytes is a real bound on shared memory" + ); + } } } + #[test] + fn corrupted_dmabuf_with_zero_chunk_size_is_rejected() { + let frame = Bounds { + chunk_size: 0, + chunk_flags: 1, // SPA_CHUNK_FLAG_CORRUPTED + ..Bounds::dmabuf_1080p() + }; + assert_eq!(frame.reason(), "producer-marked-frame-corrupted"); + } + /// stride * height is computed on widened operands because both come from /// another process; multiplied as int32 these wrap to a small positive /// number and the frame would be accepted.