[pulseaudio-discuss] [PATCH v3 08/11] pulsecore: Specially mark global mempools
Ahmed S. Darwish
darwish.07 at gmail.com
Sat Mar 12 23:09:39 UTC 2016
Color global mempools with a special mark. This special marking
is needed for handling memfd-backed pools.
To avoid fd leaks, memfd pools are registered with the connection
pstream to create an ID<->memfd mapping on both PA endpoints.
Such memory regions are then always referenced by their IDs and
never by their fds, and so their fds can be safely closed later.
Unfortunately this scheme cannot work with global pools since the
registration ID<->memfd mechanism needs to happen for each newly
connected client, and thus the need for a more special handling.
That is, for the pool's fd to be always open :-(
Almost all mempools are now created on a per-client basis. The
only exception is the pa_core's mempool which is still shared
between all clients of the system.
Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
---
src/pulse/context.c | 4 +-
src/pulsecore/core.c | 4 +-
src/pulsecore/memblock.c | 98 ++++++++++++++++++++++++++++++++++++++++-
src/pulsecore/memblock.h | 7 ++-
src/pulsecore/protocol-native.c | 2 +-
src/pulsecore/shm.h | 6 ++-
src/tests/cpu-mix-test.c | 2 +-
src/tests/lfe-filter-test.c | 2 +-
src/tests/mcalign-test.c | 2 +-
src/tests/memblock-test.c | 6 +--
src/tests/memblockq-test.c | 2 +-
src/tests/mix-test.c | 2 +-
src/tests/remix-test.c | 2 +-
src/tests/resampler-test.c | 2 +-
src/tests/srbchannel-test.c | 2 +-
15 files changed, 124 insertions(+), 19 deletions(-)
diff --git a/src/pulse/context.c b/src/pulse/context.c
index e4716fb..b8f51a6 100644
--- a/src/pulse/context.c
+++ b/src/pulse/context.c
@@ -172,11 +172,11 @@ pa_context *pa_context_new_with_proplist(pa_mainloop_api *mainloop, const char *
c->srb_template.writefd = -1;
type = !c->conf->disable_shm ? PA_MEM_TYPE_SHARED_POSIX : PA_MEM_TYPE_PRIVATE;
- if (!(c->mempool = pa_mempool_new(type, c->conf->shm_size))) {
+ if (!(c->mempool = pa_mempool_new(type, c->conf->shm_size, true))) {
if (!c->conf->disable_shm) {
pa_log_warn("Failed to allocate shared memory pool. Falling back to a normal private one.");
- c->mempool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, c->conf->shm_size);
+ c->mempool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, c->conf->shm_size, true);
}
if (!c->mempool) {
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index 199a26b..56cbe9d 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -69,14 +69,14 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, size_t shm_size) {
pa_assert(m);
if (shared) {
- if (!(pool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, shm_size))) {
+ if (!(pool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, shm_size, false))) {
pa_log_warn("Failed to allocate shared memory pool. Falling back to a normal memory pool.");
shared = false;
}
}
if (!shared) {
- if (!(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, shm_size))) {
+ if (!(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, shm_size, false))) {
pa_log("pa_mempool_new() failed.");
return NULL;
}
diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
index cdbc75a..8a7f5f3 100644
--- a/src/pulsecore/memblock.c
+++ b/src/pulsecore/memblock.c
@@ -185,6 +185,9 @@ struct pa_mempool {
pa_mutex *mutex;
pa_shm memory;
+
+ bool global;
+
size_t block_size;
unsigned n_blocks;
bool is_remote_writable;
@@ -795,7 +798,34 @@ static void memblock_replace_import(pa_memblock *b) {
pa_mutex_unlock(import->mutex);
}
-pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size) {
+/*@per_client: This is a security measure. By default this should
+ * be set to true where the created mempool is never shared with more
+ * than one client in the system. Set this to false if a global
+ * mempool, shared with all existing and future clients, is required.
+ *
+ * NOTE-1: Do not create any further global mempools! They allow data
+ * leaks between clients and thus conflict with the xdg-app containers
+ * model. They also complicate the handling of memfd-based pools.
+ *
+ * NOTE-2: Almost all mempools are now created on a per client basis.
+ * The only exception is the pa_core's mempool which is still shared
+ * between all clients of the system.
+ *
+ * Beside security issues, special marking for global mempools is
+ * required for memfd communication. To avoid fd leaks, memfd pools
+ * are registered with the connection pstream to create an ID<->memfd
+ * mapping on both PA endpoints. Such memory regions are then always
+ * referenced by their IDs and never by their fds and thus their fds
+ * can be quickly closed later.
+ *
+ * Unfortunately this scheme cannot work with global pools since the
+ * ID registration mechanism needs to happen for each newly connected
+ * client, and thus the need for a more special handling. That is,
+ * for the pool's fd to be always open :-(
+ *
+ * TODO-1: Transform the global core mempool to a per-client one
+ * TODO-2: Remove global mempools support */
+pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size, bool per_client) {
pa_mempool *p;
char t1[PA_BYTES_SNPRINT_MAX], t2[PA_BYTES_SNPRINT_MAX];
@@ -827,6 +857,8 @@ pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size) {
pa_bytes_snprint(t2, sizeof(t2), (unsigned) (p->n_blocks * p->block_size)),
(unsigned long) pa_mempool_block_size_max(p));
+ p->global = !per_client;
+
pa_atomic_store(&p->n_init, 0);
PA_LLIST_HEAD_INIT(pa_memimport, p->imports);
@@ -986,6 +1018,70 @@ void pa_mempool_unref(pa_mempool *p) {
mempool_free(p);
}
+/* No lock necessary
+ * Check pa_mempool_new() for per-client vs. global mempools */
+bool pa_mempool_is_global(pa_mempool *p) {
+ pa_assert(p);
+
+ return p->global;
+}
+
+/* No lock necessary
+ * Check pa_mempool_new() for per-client vs. global mempools */
+bool pa_mempool_is_per_client(pa_mempool *p) {
+ return !pa_mempool_is_global(p);
+}
+
+/* Self-locked
+ *
+ * This is only for per-client mempools!
+ *
+ * After this method's return, the caller owns the file descriptor
+ * and is responsible for closing it in the appropriate time. This
+ * should only be called once during during a mempool's lifetime.
+ *
+ * Check pa_shm->fd and pa_mempool_new() for further context. */
+int pa_mempool_take_memfd_fd(pa_mempool *p) {
+ int memfd_fd;
+
+ pa_assert(p);
+ pa_assert(pa_mempool_is_shared(p));
+ pa_assert(pa_mempool_is_memfd_backed(p));
+ pa_assert(pa_mempool_is_per_client(p));
+
+ pa_mutex_lock(p->mutex);
+
+ memfd_fd = p->memory.fd;
+ p->memory.fd = -1;
+
+ pa_mutex_unlock(p->mutex);
+
+ pa_assert(memfd_fd != -1);
+ return memfd_fd;
+}
+
+/* No lock necessary
+ *
+ * This is only for global mempools!
+ *
+ * Global mempools have their memfd descriptor always open. DO NOT
+ * close the returned descriptor by your own.
+ *
+ * Check pa_mempool_new() for further context. */
+int pa_mempool_get_memfd_fd(pa_mempool *p) {
+ int memfd_fd;
+
+ pa_assert(p);
+ pa_assert(pa_mempool_is_shared(p));
+ pa_assert(pa_mempool_is_memfd_backed(p));
+ pa_assert(pa_mempool_is_global(p));
+
+ memfd_fd = p->memory.fd;
+ pa_assert(memfd_fd != -1);
+
+ return memfd_fd;
+}
+
/* For receiving blocks from other nodes */
pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void *userdata) {
pa_memimport *i;
diff --git a/src/pulsecore/memblock.h b/src/pulsecore/memblock.h
index de93e3d..57ae4b2 100644
--- a/src/pulsecore/memblock.h
+++ b/src/pulsecore/memblock.h
@@ -124,7 +124,7 @@ pa_mempool * pa_memblock_get_pool(pa_memblock *b);
pa_memblock *pa_memblock_will_need(pa_memblock *b);
/* The memory block manager */
-pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size);
+pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size, bool per_client);
void pa_mempool_unref(pa_mempool *p);
pa_mempool* pa_mempool_ref(pa_mempool *p);
const pa_mempool_stat* pa_mempool_get_stat(pa_mempool *p);
@@ -132,10 +132,15 @@ void pa_mempool_vacuum(pa_mempool *p);
int pa_mempool_get_shm_id(pa_mempool *p, uint32_t *id);
bool pa_mempool_is_shared(pa_mempool *p);
bool pa_mempool_is_memfd_backed(const pa_mempool *p);
+bool pa_mempool_is_global(pa_mempool *p);
+bool pa_mempool_is_per_client(pa_mempool *p);
bool pa_mempool_is_remote_writable(pa_mempool *p);
void pa_mempool_set_is_remote_writable(pa_mempool *p, bool writable);
size_t pa_mempool_block_size_max(pa_mempool *p);
+int pa_mempool_take_memfd_fd(pa_mempool *p);
+int pa_mempool_get_memfd_fd(pa_mempool *p);
+
/* For receiving blocks from other nodes */
pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void *userdata);
void pa_memimport_free(pa_memimport *i);
diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
index afb4850..c9182d0 100644
--- a/src/pulsecore/protocol-native.c
+++ b/src/pulsecore/protocol-native.c
@@ -2622,7 +2622,7 @@ static void setup_srbchannel(pa_native_connection *c) {
return;
}
- if (!(c->rw_mempool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, c->protocol->core->shm_size))) {
+ if (!(c->rw_mempool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, c->protocol->core->shm_size, true))) {
pa_log_warn("Disabling srbchannel, reason: Failed to allocate shared "
"writable memory pool.");
return;
diff --git a/src/pulsecore/shm.h b/src/pulsecore/shm.h
index e8bfa56..67a2114 100644
--- a/src/pulsecore/shm.h
+++ b/src/pulsecore/shm.h
@@ -41,7 +41,11 @@ typedef struct pa_shm {
*
* When we don't have ownership for the memfd fd in question (e.g.
* pa_shm_attach()), or the file descriptor has now been closed,
- * this is set to -1. */
+ * this is set to -1.
+ *
+ * For the special case of a global mempool, we keep this fd
+ * always open. Check comments on top of pa_mempool_new() for
+ * rationale. */
int fd;
} pa_shm;
diff --git a/src/tests/cpu-mix-test.c b/src/tests/cpu-mix-test.c
index e487a20..a3189b7 100644
--- a/src/tests/cpu-mix-test.c
+++ b/src/tests/cpu-mix-test.c
@@ -76,7 +76,7 @@ static void run_mix_test(
samples_ref = out_ref + (8 - align);
nsamples = channels * (SAMPLES - (8 - align));
- fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0)) != NULL, NULL);
+ fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true)) != NULL, NULL);
pa_random(samples0, nsamples * sizeof(int16_t));
c0.memblock = pa_memblock_new_fixed(pool, samples0, nsamples * sizeof(int16_t), false);
diff --git a/src/tests/lfe-filter-test.c b/src/tests/lfe-filter-test.c
index 8d61cc6..ba63895 100644
--- a/src/tests/lfe-filter-test.c
+++ b/src/tests/lfe-filter-test.c
@@ -136,7 +136,7 @@ START_TEST (lfe_filter_test) {
a.format = PA_SAMPLE_S16NE;
lft.ss = &a;
- pa_assert_se(lft.pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));
+ pa_assert_se(lft.pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true));
/* We prepare pseudo-random input audio samples for lfe-filter rewind testing*/
ori_sample_ptr = pa_xmalloc(pa_frame_size(lft.ss) * TOTAL_SAMPLES);
diff --git a/src/tests/mcalign-test.c b/src/tests/mcalign-test.c
index f121972..3127ccf 100644
--- a/src/tests/mcalign-test.c
+++ b/src/tests/mcalign-test.c
@@ -36,7 +36,7 @@ int main(int argc, char *argv[]) {
pa_mcalign *a;
pa_memchunk c;
- p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0);
+ p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true);
a = pa_mcalign_new(11);
diff --git a/src/tests/memblock-test.c b/src/tests/memblock-test.c
index 04eb377..d48349f 100644
--- a/src/tests/memblock-test.c
+++ b/src/tests/memblock-test.c
@@ -81,11 +81,11 @@ START_TEST (memblock_test) {
const char txt[] = "This is a test!";
- pool_a = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
+ pool_a = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0, true);
fail_unless(pool_a != NULL);
- pool_b = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
+ pool_b = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0, true);
fail_unless(pool_b != NULL);
- pool_c = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
+ pool_c = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0, true);
fail_unless(pool_c != NULL);
pa_mempool_get_shm_id(pool_a, &id_a);
diff --git a/src/tests/memblockq-test.c b/src/tests/memblockq-test.c
index 37cdd77..2404ee2 100644
--- a/src/tests/memblockq-test.c
+++ b/src/tests/memblockq-test.c
@@ -108,7 +108,7 @@ START_TEST (memblockq_test) {
pa_log_set_level(PA_LOG_DEBUG);
- p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0);
+ p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true);
silence.memblock = pa_memblock_new_fixed(p, (char*) "__", 2, 1);
fail_unless(silence.memblock != NULL);
diff --git a/src/tests/mix-test.c b/src/tests/mix-test.c
index ce6686a..972f5d7 100644
--- a/src/tests/mix-test.c
+++ b/src/tests/mix-test.c
@@ -286,7 +286,7 @@ START_TEST (mix_test) {
if (!getenv("MAKE_CHECK"))
pa_log_set_level(PA_LOG_DEBUG);
- fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0)) != NULL, NULL);
+ fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true)) != NULL, NULL);
a.channels = 1;
a.rate = 44100;
diff --git a/src/tests/remix-test.c b/src/tests/remix-test.c
index 6ee5f6d..e21c109 100644
--- a/src/tests/remix-test.c
+++ b/src/tests/remix-test.c
@@ -51,7 +51,7 @@ int main(int argc, char *argv[]) {
pa_log_set_level(PA_LOG_DEBUG);
- pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));
+ pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true));
for (i = 0; maps[i].channels > 0; i++)
for (j = 0; maps[j].channels > 0; j++) {
diff --git a/src/tests/resampler-test.c b/src/tests/resampler-test.c
index 28f03d6..40000d5 100644
--- a/src/tests/resampler-test.c
+++ b/src/tests/resampler-test.c
@@ -404,7 +404,7 @@ int main(int argc, char *argv[]) {
}
ret = 0;
- pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));
+ pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0, true));
if (!all_formats) {
diff --git a/src/tests/srbchannel-test.c b/src/tests/srbchannel-test.c
index 253abcf..9fc5d45 100644
--- a/src/tests/srbchannel-test.c
+++ b/src/tests/srbchannel-test.c
@@ -85,7 +85,7 @@ START_TEST (srbchannel_test) {
int pipefd[4];
pa_mainloop *ml = pa_mainloop_new();
- pa_mempool *mp = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
+ pa_mempool *mp = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0, true);
pa_iochannel *io1, *io2;
pa_pstream *p1, *p2;
pa_srbchannel *sr1, *sr2;
--
2.7.2
More information about the pulseaudio-discuss
mailing list