On 2016-02-12 00:59, Ahmed S. Darwish wrote: > Hello! Hi and thanks for working on it! Having skimmed through the patches, I'm still not convinced w r t auto-send mechanism of the memfds. E g, consider we have clients 1 and 2, and client 1 plays back audio to a sink. Client 2 monitors the sink-input of client 1 (like parec does if you specify --monitor-stream). Could this cause the memfd used to communicate with client 1 to also be shared with client 2? If so, that's a security breach. The mechanism seems fragile to such breaches but I'm not sure, maybe I'm wrong and you can explain what would happen instead? > > The all-improved memfd patch series ;-) > > ==> v1 cover letter: > > Memfd is a simple memory sharing mechanism, added by the systemd/kdbus > developers, to share pages between processes in an anonymous, no > global registry needed, no mount-point required, relatively secure, > manner. > > This patch series introduces memfd support to PulseAudio, laying out > the necessary (but not yet sufficient) groundwork for sandboxing, > protecting PulseAudio from its clients, and protecting clients from > each other. > > Memfd support is added in quite a transparent manner, respecting > current PA mechanisms and abstractions. The lower-level layers are > properly refactored and extended: the srbchannel communication path > is transformed to memfds by only changing a single line of code. > > ==> v2 changes: > > - All pools (srbchannel + client audio data mempool + global mempool) > now use memfds by default. An empty /dev/shm FTW! ;-) > > - Design issues pointed by David now fixed, leading to a much smaller > code.. New memfd implementation now shares almost all of the > existing posix SHM code paths. > > - Heavy focus on not leaking any file descriptors throughout the > code. Every fd, sent and received, has a clear owner responsible > for closing it at appropriate times. > > - If a mempool is memfd-backed, we now require registering it with > the pstream before sending any blocks it covers. This is done to > to minimize fd passing overhead and the possibility of fd leaks. > > - A new, special, pstream packet type is introduced. That special > packet can be sent anytime during the pstrem's lifetime and is > used for creating on demand SHM ID to memfd mappings. > > - All issues pointed by Tanu's very detailed review (vacuuming, > memfd enablement over pstream, protocol changes, etc.) now fixed > > - Testing! I've been heavily using these patches now for three > weeks without visible issues. > > ==> references: > > - v1 submission > http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/24110 > > - memfd_create(2), David Herrmann [memfd author] > https://dvdhrm.wordpress.com/2014/06/10/memfd_create2/ > > - xdg-app > https://wiki.gnome.org/Projects/SandboxedApps > > ==> diffstat: > > Ahmed S. Darwish (12): > pulsecore: Cache daemon shm size inside pa_core > pulsecore: srbchannel: Introduce per-client SHM files > pulsecore: Transform pa_mempool_new() into a factory method > pulsecore: Split pa_shm mempool backend into pa_shm and pa_privatemem > pulsecore: SHM: Introduce memfd support > pulsecore: memexport/memimport: Support memfd blocks > pulsecore, tests: Use new memexport/memimport API > pulsecore: Specially mark global mempools > pulsecore: pstream: Support memfd blocks transport > srbchannel: Use memfd transport by default; pump protocol version > pulse: client audio: Use memfd transport by default > core: Use memfd transport by default > > PROTOCOL | 13 ++ > configure.ac | 21 +++- > man/pulse-client.conf.5.xml.in | 5 + > man/pulse-daemon.conf.5.xml.in | 5 + > man/pulseaudio.1.xml.in | 11 ++ > shell-completion/bash/pulseaudio | 4 +- > shell-completion/zsh/_pulseaudio | 1 + > src/Makefile.am | 7 ++ > src/daemon/cmdline.c | 13 +- > src/daemon/daemon-conf.c | 1 + > src/daemon/daemon-conf.h | 1 + > src/daemon/main.c | 4 +- > src/pulse/client-conf.c | 1 + > src/pulse/client-conf.h | 2 +- > src/pulse/context.c | 48 +++++++- > src/pulse/internal.h | 2 + > src/pulsecore/core.c | 31 +++-- > src/pulsecore/core.h | 13 +- > src/pulsecore/mem.h | 67 ++++++++++ > src/pulsecore/memblock.c | 245 ++++++++++++++++++++++++++++++++----- > src/pulsecore/memblock.h | 18 ++- > src/pulsecore/memfd-wrappers.h | 68 +++++++++++ > src/pulsecore/privatemem.c | 78 ++++++++++++ > src/pulsecore/privatemem.h | 35 ++++++ > src/pulsecore/protocol-native.c | 92 ++++++++++++-- > src/pulsecore/pstream.c | 257 +++++++++++++++++++++++++++++++++++---- > src/pulsecore/pstream.h | 2 + > src/pulsecore/shm.c | 247 +++++++++++++++++++------------------ > src/pulsecore/shm.h | 33 ++++- > 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 | 15 +-- > 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 +- > 38 files changed, 1119 insertions(+), 237 deletions(-) > create mode 100644 src/pulsecore/mem.h > create mode 100644 src/pulsecore/memfd-wrappers.h > create mode 100644 src/pulsecore/privatemem.c > create mode 100644 src/pulsecore/privatemem.h > > Regards, > > -- > Darwish > http://darwish.chasingpointers.com > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
