[pulseaudio-discuss] [PATCH] protocol-native: Fix srbchannel double-freeing

[David Henningsson]

On 2014-09-14 21:06, Tanu Kaskinen wrote:
> I was able to trigger a crash with some unfinished code that caused
> a protocol error. The error was handled while
> pa_pstream_set_srbchannel() had not yet finished, so
> pa_native_connection.srbpending and pa_pstream.srbpending were both
> non-NULL. During the error handling, native_connection_unlink() freed
> the srbchannel, and then pa_pstream_unlink() tried to free it for the
> second time, causing an assertion error in mainloop_io_free().

Ok, thanks for finding!

> ---
>   src/pulsecore/protocol-native.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index 6ec65d6..012c41a 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -2639,13 +2639,25 @@ static void setup_srbchannel(pa_native_connection *c) {
>
>   static void command_enable_srbchannel(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
>       pa_native_connection *c = PA_NATIVE_CONNECTION(userdata);
> +    pa_srbchannel *tmp;
>
>       if (tag != (uint32_t) (size_t) c->srbpending)
>           protocol_error(c);
>
>       pa_log_debug("Client enabled srbchannel.");
> -    pa_pstream_set_srbchannel(c->pstream, c->srbpending);
> +
> +    /* We must set c->sbpending to NULL before calling
> +     * pa_pstream_set_srbchannel(), because otherwise there is a time window
> +     * when both pa_native_connection and pa_pstream think they own the
> +     * srbchannel. pa_pstream_set_srbchannel() may read and dispatch arbitrary
> +     * commands from the client, and if a protocol error occurs, the connection
> +     * is torn down, and if c->sbpending has not yet been set to NULL here, the
> +     * srbchannel will be double-freed. XXX: Can we somehow avoid reading and
> +     * dispatching commands in pa_pstream_set_srbchannel()? Processing commands
> +     * recursively seems prone to bugs. */
> +    tmp = c->srbpending;
>       c->srbpending = NULL;
> +    pa_pstream_set_srbchannel(c->pstream, c->srbpending);

You probably mean "tmp" here instead of "c->srbpending"...

Anyhow, I think it would be better to try to resolve your concern by 
implementing a deferred event. Because, as you say, processing commands 
recursive seems prone to bugs. I e, pa_srbchannel_set_callback would not 
call srbchannel_rwloop, but instead set up a deferred event which would 
just call srbchannel_rwloop.

Does that make sense?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic