[pulseaudio-discuss] [PATCH v2 02/12] pulsecore: srbchannel: Introduce per-client SHM files

[Ahmed S. Darwish]

On Fri, Feb 12, 2016 at 02:55:32PM +0100, David Henningsson wrote:
> 
> 
> On 2016-02-12 01:09, Ahmed S. Darwish wrote:
> >The PA daemon currently uses a single SHM file for all the clients
> >sending and receiving commands over the low-latency srbchannel
> >mechanism.
> >
> >To safely run PA daemon in system mode later using memfds, and to
> >provide the necessary ground work for sandboxing, create the
> >srbchannel SHM files on a per-client basis.
> >
> >Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> >---
> >  src/pulsecore/core.c            | 19 ++++++++++---------
> >  src/pulsecore/core.h            |  6 ++----
> >  src/pulsecore/protocol-native.c | 28 ++++++++++++++++++++++++----
> >  3 files changed, 36 insertions(+), 17 deletions(-)
> >
> >[ For more details on the vacuuming issue, please check the
> >   message sent in direct reply to this patch.. ]
> 
> If the srbchannel is the only thing present in that pool, then vacuuming
> does not even make sense?
>

Hmm... The code modified also vacuumed the rw mempool, which was
used exclusively for srbchannels.

But looking at it now, it makes sense specially not to vacuum the
per-client version of it. All of what is used is a single 4K page
for the shared ringbuffer anyway.

> >@@ -1348,6 +1354,12 @@ static void native_connection_unlink(pa_native_connection *c) {
> >      if (c->pstream)
> >          pa_pstream_unlink(c->pstream);
> >
> >+    /* This mempool is used exclusively by srbchanels. Do not free it
> >+     * except after ending all srbchannel communications through the
> >+     * pstreams unlink above */
> >+    if (c->rw_mempool)
> >+        pa_mempool_free(c->rw_mempool);
> 
> A general convention for _unlink functions is that they should allow being
> called more than one time. So all places which free the mempool should also
> set the variable to NULL.
>

ACK.

> >+
> >      if (c->auth_timeout_event) {
> >          c->protocol->core->mainloop->time_free(c->auth_timeout_event);
> >          c->auth_timeout_event = NULL;
> >@@ -2606,15 +2618,17 @@ static void setup_srbchannel(pa_native_connection *c) {
> >          return;
> >      }
> >
> >-    if (!c->protocol->core->rw_mempool) {
> >-        pa_log_debug("Disabling srbchannel, reason: No rw memory pool");
> 
> What if c->rw_mempool is already set at this point? Can that happen? If not,
> add an assertion, if it can (consider a malicious client) ...then handle it
> somehow :-)
>

Hmm.. malicious client sending multiple PA_COMMAND_AUTH. Didn't
think of that before :-) I'll see how to gracefully handle this.

> >+    if (!(c->rw_mempool = pa_mempool_new(true, c->protocol->core->shm_size))) {
> >+        pa_log_warn("Disabling srbchannel, reason: Failed to allocate shared "
> >+                    "writable memory pool.");
> >          return;
> >      }
> >+    pa_mempool_set_is_remote_writable(c->rw_mempool, true);
> >
> >-    srb = pa_srbchannel_new(c->protocol->core->mainloop, c->protocol->core->rw_mempool);
> >+    srb = pa_srbchannel_new(c->protocol->core->mainloop, c->rw_mempool);
> >      if (!srb) {
> >          pa_log_debug("Failed to create srbchannel");
> >-        return;
> >+        goto free_rw_mempool;
> 
> If this is the only place you need goto, just instead do
> 
> 	pa_mempool_free(c->rw_mempool);
> 	c->rw_mempool = NULL;
> 	return;
>

ACK.

> >      }
> >      pa_log_debug("Enabling srbchannel...");
> >      pa_srbchannel_export(srb, &srbt);
> >@@ -2634,6 +2648,10 @@ static void setup_srbchannel(pa_native_connection *c) {
> >      pa_pstream_send_memblock(c->pstream, 0, 0, 0, &mc);
> >
> >      c->srbpending = srb;
> >+    return;
> >+
> >+free_rw_mempool:
> >+    pa_mempool_free(c->rw_mempool);
> >  }
> >
> >  static void command_enable_srbchannel(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> >@@ -5125,6 +5143,8 @@ void pa_native_protocol_connect(pa_native_protocol *p, pa_iochannel *io, pa_nati
> >      c->client->send_event = client_send_event_cb;
> >      c->client->userdata = c;
> >
> >+    c->rw_mempool = NULL;
> 
> A preferred way nowadays is to instead use pa_xnew0 when creating the struct
> initially.
>

ACK.

Thanks a lot,

-- 
Darwish
http://darwish.chasingpointers.com