From 0b08468035f1a4194dbb53bc3d60d6077f011be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barnab=C3=A1s=20P=C5=91cze?= Date: Mon, 8 Sep 2025 12:47:10 +0200 Subject: [PATCH] pipewire: mem: pw_memblock_map(): fix pointer when reusing mapping Previously the pointer was determined as follows: mm->this.ptr = SPA_PTROFF(m->ptr, range.start, void); however, when `pw_map_range` is calculated, `pw_map_range::start` is the offset from the beginning of the first page, starting at `pw_map_range::offset`. This works correctly if `memblock_map()` runs because that will map the file with expected offset, so using `range.start` is correct. However, when a mapping is reused (i.e. `memblock_find_mapping()`) finds something, then `range.start` is not necessarily correct. Consider the following example: * page size is 10 * one memblock with size 20 (2 pages) * the applications wants to mappings: * (offset=5,size=10) * (offset=15,size=5) After the first request from the application, a `mapping` object is created that covers the first two pages of the memblock: offset=0 and size=20. During the second request, the calculated `pw_map_range` is as follows: { start = 5, offset = 10, size = 10 } and the only previously created mapping is reused since (0 <= 5) and (10 <= 20). When the pointer of the mapping is adjusted afterwards it will be incorrect since `m->ptr` points to byte 0 on page 0 (instead of byte 0 on page 1 -- that is assumed). Thereforce the two will unexpectedly overlap. Fix that by using `offset - m->offset` when adjusting the mapping's pointer. Also move the `range` variable into a smaller scope because it only makes sense there. And add a test that check the above previously incorrect case. Fixes: 2caf81c97c0aee ("mem: improve memory handling") Fixes #4884 --- src/pipewire/mem.c | 15 +++++++------- test/meson.build | 1 + test/test-mempool.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 test/test-mempool.c diff --git a/src/pipewire/mem.c b/src/pipewire/mem.c index 1fa371247..642a6b78e 100644 --- a/src/pipewire/mem.c +++ b/src/pipewire/mem.c @@ -393,7 +393,6 @@ struct pw_memmap * pw_memblock_map(struct pw_memblock *block, struct mempool *p = SPA_CONTAINER_OF(block->pool, struct mempool, this); struct mapping *m; struct memmap *mm; - struct pw_map_range range; struct stat sb; if (b->this.fd == -1) { @@ -418,13 +417,15 @@ struct pw_memmap * pw_memblock_map(struct pw_memblock *block, return NULL; } - pw_map_range_init(&range, offset, size, p->pagesize); - m = memblock_find_mapping(b, flags, offset, size); - if (m == NULL) + if (m == NULL) { + struct pw_map_range range; + pw_map_range_init(&range, offset, size, p->pagesize); + m = memblock_map(b, flags, range.offset, range.size); - if (m == NULL) - return NULL; + if (m == NULL) + return NULL; + } mm = calloc(1, sizeof(struct memmap)); if (mm == NULL) { @@ -439,7 +440,7 @@ struct pw_memmap * pw_memblock_map(struct pw_memblock *block, mm->this.flags = flags; mm->this.offset = offset; mm->this.size = size; - mm->this.ptr = SPA_PTROFF(m->ptr, range.start, void); + mm->this.ptr = SPA_PTROFF(m->ptr, offset - m->offset, void); pw_log_debug("%p: map:%p block:%p fd:%d flags:%08x ptr:%p (%u %u) mapping:%p ref:%d", p, &mm->this, b, b->this.fd, b->this.flags, mm->this.ptr, offset, size, m, m->ref); diff --git a/test/meson.build b/test/meson.build index c5671149d..ad658bcc6 100644 --- a/test/meson.build +++ b/test/meson.build @@ -52,6 +52,7 @@ test('test-pw-utils', 'test-properties.c', 'test-array.c', 'test-map.c', + 'test-mempool.c', 'test-utils.c', include_directories: pwtest_inc, dependencies: [ spa_dep ], diff --git a/test/test-mempool.c b/test/test-mempool.c new file mode 100644 index 000000000..36f69c571 --- /dev/null +++ b/test/test-mempool.c @@ -0,0 +1,49 @@ +/* PipeWire */ +/* SPDX-FileCopyrightText: Copyright © 2025 PipeWire authors */ +/* SPDX-License-Identifier: MIT */ + +#include + +#include +#include + +#include "pwtest.h" + +PWTEST(mempool_issue4884) +{ + /* + * See https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/4884. This + * test checks if the offset is correctly applied when a mapping is reused. + */ + + long page_size = sysconf(_SC_PAGESIZE); + pwtest_errno_ok(page_size); + pwtest_int_ge(page_size, 8); + + struct pw_mempool *p = pw_mempool_new(NULL); + pwtest_ptr_notnull(p); + + struct pw_memblock *b = pw_mempool_alloc(p, PW_MEMBLOCK_FLAG_READWRITE, SPA_DATA_MemFd, 2 * page_size); + pwtest_ptr_notnull(b); + + struct pw_memmap *m1 = pw_mempool_map_id(p, b->id, PW_MEMMAP_FLAG_READWRITE, page_size / 2, page_size, NULL); + pwtest_ptr_notnull(m1); + pwtest_ptr_eq(m1->block, b); + + struct pw_memmap *m2 = pw_mempool_map_id(p, b->id, PW_MEMMAP_FLAG_READWRITE, 3 * page_size / 2, page_size / 2, NULL); + pwtest_ptr_notnull(m2); + pwtest_ptr_eq(m2->block, b); + + pwtest_int_eq(SPA_PTRDIFF(m2->ptr, m1->ptr), page_size); + + pw_mempool_destroy(p); + + return PWTEST_PASS; +} + +PWTEST_SUITE(pw_mempool) +{ + pwtest_add(mempool_issue4884, PWTEST_NOARG); + + return PWTEST_PASS; +}