[Spice-devel] [PATCH 2/2] audio: Avoid bad pipelines from gst_parse_launch

[Fabiano Fidêncio]

On Tue, 2014-11-25 at 17:25 +0100, Victor Toso wrote:
> gst_parse_launch may return not NULL even when error is set.
> This can lead to data loss when playing or recording audio.
> ---
>  gtk/spice-gstaudio.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/gtk/spice-gstaudio.c b/gtk/spice-gstaudio.c
> index 5f9abb2..d6cc774 100644
> --- a/gtk/spice-gstaudio.c
> +++ b/gtk/spice-gstaudio.c
> @@ -245,7 +245,7 @@ static void record_start(SpiceRecordChannel *channel, gint format, gint channels
>                              "appsink caps=\"%s\" name=appsink", audio_caps);
>  
>          p->record.pipe = gst_parse_launch(pipeline, &error);
> -        if (p->record.pipe == NULL) {
> +        if (error != NULL) {
>              g_warning("Failed to create pipeline: %s", error->message);
>              goto lerr;
>          }
> @@ -269,6 +269,10 @@ static void record_start(SpiceRecordChannel *channel, gint format, gint channels
>  #endif
>  
>  lerr:
> +        if (error != NULL && p->record.pipe != NULL) {
> +            gst_object_unref(p->record.pipe);
> +            p->record.pipe = NULL;
> +        }

It's kind-of related to your code here ...
I don't see a reason to have the goto and this lerr label here.
- audio_caps could only be set when the pipeline is NULL and freed right
inside this context.
- pipeline could be freed just after gst_parse_launch() and right before
the error checking.
- the error checking could cleanup the error and pipe inside the if and
simply return here. 


>          g_clear_error(&error);
>          g_free(audio_caps);
>          g_free(pipeline);
> @@ -345,7 +349,7 @@ static void playback_start(SpicePlaybackChannel *channel, gint format, gint chan
>                                         "audioconvert ! audioresample ! autoaudiosink name=\"audiosink\"", audio_caps);
>          SPICE_DEBUG("audio pipeline: %s", pipeline);
>          p->playback.pipe = gst_parse_launch(pipeline, &error);
> -        if (p->playback.pipe == NULL) {
> +        if (error != NULL) {
>              g_warning("Failed to create pipeline: %s", error->message);
>              goto lerr;
>          }
> @@ -355,6 +359,10 @@ static void playback_start(SpicePlaybackChannel *channel, gint format, gint chan
>          p->playback.channels = channels;
>  
>  lerr:
> +        if (error != NULL && p->playback.pipe != NULL) {
> +            gst_object_unref(p->playback.pipe);
> +            p->playback.pipe = NULL;
> +        }
>          g_clear_error(&error);
>          g_free(audio_caps);
>          g_free(pipeline);

The comments more or less apply to this hunk as well. But here I think a
goto exit would make sense. The label exit, however, should be in the
line 373 (just before the if (p->mmtime_id ...))

Best Regards,
--
Fabiano Fidêncio