[pulseaudio-discuss] [PATCH 0/2] modules, pulse, core: React to failing pa_memblockq_push calls.

[Tanu Kaskinen]

On Thu, 2012-05-24 at 10:38 +0300, Jarkko Suontausta wrote:
> Based on Coverity findings, the return values of pa_memblockq_push and
> pa_memblockq_push_align calls were not taken into account. These patches
> are addressing many of those cases by adding an assertion.
> 
> Thanks to a thorough review of the first version, Tanu is already familiar
> with the patches. He had many good proposals for the remaining FIXME cases,
> but unfortunately I do not have time to address them now.

For reference to other reviewers and also to myself, below is the first
review (for the second patch - the first one looks good to me as is):


On Tue, 2012-04-24 at 11:55 +0300, Jarkko Suontausta wrote:
> diff --git a/src/modules/module-loopback.c b/src/modules/module-loopback.c
> index ba62453..aef0880 100644
> --- a/src/modules/module-loopback.c
> +++ b/src/modules/module-loopback.c
> @@ -461,7 +461,7 @@ static int sink_input_process_msg_cb(pa_msgobject *obj, int code, void *data, in
>              pa_sink_input_assert_io_context(u->sink_input);
>  
>              if (PA_SINK_IS_OPENED(u->sink_input->sink->thread_info.state))
> -                pa_memblockq_push_align(u->memblockq, chunk);
> +                pa_assert_se(pa_memblockq_push_align(u->memblockq, chunk) >= 0);
>              else
>                  pa_memblockq_flush_write(u->memblockq, TRUE);

This may not be safe if the source is very fast or the sink is very
slow. Maybe unloading the module would be the best way to handle an
error situation here?

> diff --git a/src/pulse/context.c b/src/pulse/context.c
> index 5bd3448..0ca3ebf 100644
> --- a/src/pulse/context.c
> +++ b/src/pulse/context.c
> @@ -354,6 +354,7 @@ static void pstream_memblock_callback(pa_pstream *p, uint32_t channel, int64_t o
>  
>          if (chunk->memblock) {
>              pa_memblockq_seek(s->record_memblockq, offset, seek, TRUE);
> +            /* FIXME: pa_memblockq_push_align()'s return value is not checked */
>              pa_memblockq_push_align(s->record_memblockq, chunk);
>          } else
>              pa_memblockq_seek(s->record_memblockq, offset+chunk->length, seek, TRUE);

It looks like record_memblockq can get full if the application reads
data slower than what the server sends it. It would be nice to notify
the application. There's pa_stream_set_overflow_callback(), but it's
only for playback streams. I think leaving this as FIXME is the best
choice.

> diff --git a/src/pulsecore/protocol-esound.c b/src/pulsecore/protocol-esound.c
> index 4e1c56c..9b2d011 100644
> --- a/src/pulsecore/protocol-esound.c
> +++ b/src/pulsecore/protocol-esound.c
> @@ -1304,7 +1304,7 @@ static int connection_process_msg(pa_msgobject *o, int code, void*userdata, int6
>  
>          case CONNECTION_MESSAGE_POST_DATA:
>  /*             pa_log("got data %u", chunk->length); */
> -            pa_memblockq_push_align(c->output_memblockq, chunk);
> +            pa_assert_se(pa_memblockq_push_align(c->output_memblockq, chunk) >= 0);
>              do_work(c);
>              break;

output_memblockq can get full if the connection's iochannel isn't able
to consume the data fast enough. I think that can happen if the client
doesn't read data fast enough - not a good reason to make Pulseaudio
crash. I don't know if esound supports buffer overflow notifications -
if not, then I'm not sure what should be done. I guess printing
something to the log would be good. If this happens too often, I think
even disconnecting the client could be an option (I guess this applies
to the previous case as well, which was a similar situation with the
native protocol).

> diff --git a/src/pulsecore/protocol-http.c b/src/pulsecore/protocol-http.c
> index d745634..41e7347 100644
> --- a/src/pulsecore/protocol-http.c
> +++ b/src/pulsecore/protocol-http.c
> @@ -207,6 +207,8 @@ static int source_output_process_msg(pa_msgobject *m, int code, void *userdata,
>          case SOURCE_OUTPUT_MESSAGE_POST_DATA:
>              /* While this function is usually called from IO thread
>               * context, this specific command is not! */
> +
> +            /* FIXME: pa_memblockq_push_align()'s return value is not checked */
>              pa_memblockq_push_align(c->output_memblockq, chunk);
>              do_work(c);
>              break;

This seems to be an equivalent situation with the output_memblockq of
protocol-esound.

> diff --git a/src/pulsecore/protocol-simple.c b/src/pulsecore/protocol-simple.c
> index 8d8f5b8..53d4807 100644
> --- a/src/pulsecore/protocol-simple.c
> +++ b/src/pulsecore/protocol-simple.c
> @@ -290,6 +290,7 @@ static int connection_process_msg(pa_msgobject *o, int code, void*userdata, int6
>  
>          case CONNECTION_MESSAGE_POST_DATA:
>  /*             pa_log("got data %u", chunk->length); */
> +            /* FIXME: pa_memblockq_push_align()'s return value is not checked */
>              pa_memblockq_push_align(c->output_memblockq, chunk);
>              do_work(c);
>              break;

Equivalent case with protocol-esound and protocol-http.

> @@ -319,6 +320,7 @@ static int sink_input_process_msg(pa_msgobject *o, int code, void *userdata, int
>              pa_assert(chunk);
>  
>              /* New data from the main loop */
> +            /* FIXME: pa_memblockq_push_align()'s return value is not checked */
>              pa_memblockq_push_align(c->input_memblockq, chunk);
>  
>              if (pa_memblockq_is_readable(c->input_memblockq) && c->playback.underrun) {

do_read() doesn't read anything and consequently doesn't push anything
to the queue if c->playback.missing is 0, and if it's not 0, then at
maximum the missing amount is read/pushed. The missing amount is what
the memblockq needs to fulfill the target buffering level.

If the client writes too fast, it will eventually find that it can't
write anything to its socket anymore, because the server hasn't read the
old data yet. Therefore, the write speed gets synchronized to the read
speed and this call can be assumed to always succeed.

-- 
Tanu