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
