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

[Christophe Fergeau]

On Wed, Nov 26, 2014 at 11:18:23AM +0100, Fabiano Fidêncio wrote:
> On Wed, Nov 26, 2014 at 10:14 AM, Christophe Fergeau
> <cfergeau at redhat.com> wrote:
> > 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.
> 
> I understand the idea, but the code itself is not "clear" as it could be.
> "lerr" as label name makes me think that I'm reaching that part in
> case of error, then the error checking just after the label tells me
> the opposite.

Ah right, the naming of the label is unusual, feel free to change it to
"end:" or "cleanup:" for something clearer.

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/bc64b371/attachment.sig>