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

[Christophe Fergeau]

On Tue, Nov 25, 2014 at 11:06:05PM +0100, Fabiano Fidêncio wrote:
> 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. 

For what it's worth, grouping all cleanup code to the end of the
function avoids cluttering the main code path with too much
housekeeping, if we add more codepath which can error out, then
the new codepath will not introduce leaks of existing objects if it
errors out, ... so I'd tend to keep the code this way unless there's a
huge benefit of moving the cleanup code back to the error checking
sites.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20141126/2f102eb3/attachment.sig>