[Spice-devel] [PATCH spice-gtk 2/2] gstreamer: Avoid dangling pointers in free_pipeline

[Frediano Ziglio]

Il giorno ven 3 nov 2023 alle ore 04:58 Kasireddy, Vivek
<vivek.kasireddy at intel.com> ha scritto:
>
> Hi Frediano,
>
> >
> > Although currently after free_pipeline we free the entire structure
> > the name and the function suggests that we only free the pipeline.
> > Also this is fixing a future possible problem with the series
> > from Vivek Kasireddy reusing the SpiceGstDecoder for another
> > pipeline if H/W encoder pipeline creation fails.
> >
> > Signed-off-by: Frediano Ziglio <freddy77 at gmail.com>
> > ---
> >  src/channel-display-gst.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 3bd948d2..ffc2edbf 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -361,11 +361,14 @@ static void free_pipeline(SpiceGstDecoder
> > *decoder)
> >
> >      gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
> >      gst_object_unref(decoder->appsrc);
> > +    decoder->appsrc = NULL;
> >      if (decoder->appsink) {
> Would it make sense to get rid of the above if as well to be consistent?
> I don't see why it is needed given that gst_object_unref can accept NULL
> as input. Regardless, this patch is
> Acked-by: Vivek Kasireddy <vivek.kasireddy at intel.com>


If you pass NULL it gives a warning which we don't want (free and
g_free are different in this respect).
Wondering if it won't be better or more consistent to always check for NULL.
In this case the pipeline can be fully constructed but appsink can be
NULL in case we arrange to have GStreamer writing directly on video
using an overlay.

Thanks for the review.

>
> Thanks,
> Vivek
>
> >          gst_object_unref(decoder->appsink);
> > +        decoder->appsink = NULL;
> >      }
> > -    gst_object_unref(decoder->pipeline);
> >      gst_object_unref(decoder->clock);
> > +    decoder->clock = NULL;
> > +    gst_object_unref(decoder->pipeline);
> >      decoder->pipeline = NULL;
> >  }
> >

Frediano