[Spice-devel] [PATCH spice-gtk 1/2] gstreamer: Fix leak using GstBus watch

[Frediano Ziglio]

Il giorno sab 4 nov 2023 alle ore 16:22 Kasireddy, Vivek
<vivek.kasireddy at intel.com> ha scritto:
>
> Hi Frediano,
>
> >
> > This patch fixes a leak due to not freeing GstBus watch.
> > The watch is attached (as GSource) to the main loop and retains
> > a pointer to the bus so we need to remove it to release the bus
> > when we release the pipeline.
> > This was detected forcibly creating and destroying lot of streams.
> > After a while the client program consumed all file descriptors
> > and stopped working. This as GstBus retains a GPoll which,
> > under Unix, uses 2 file descriptors.
> > For some reasons using gst_pipeline_get_bus again in free_pipeline
> > do not fix entirely the leak so keep a pointer to the exact
> > bus we set our watch on.
> >
> > Signed-off-by: Frediano Ziglio <freddy77 at gmail.com>
> > ---
> >  src/channel-display-gst.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 3b372dc0..6e126000 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -47,6 +47,7 @@ typedef struct SpiceGstDecoder {
> >      GstAppSink *appsink;
> >      GstElement *pipeline;
> >      GstClock *clock;
> > +    GstBus *bus;
> >
> >      /* ---------- Decoding and display queues ---------- */
> >
> > @@ -352,6 +353,13 @@ static void free_pipeline(SpiceGstDecoder
> > *decoder)
> >          return;
> >      }
> >
> > +    GstBus *bus = decoder->bus;
> > +    if (bus) {
> > +        gst_bus_remove_watch(bus);
> > +        gst_object_unref(bus);
> > +        decoder->bus = NULL;
> > +    }
> Looks like the watch can be removed by returning FALSE from the bus
> func (handle_pipeline_message) as well. Would it make sense to handle
> it this way?
>

The problem is that if we don't get a message we are not sure the
watch is removed.
There are many cases where we could release the pipeline but receive
different events from the bus.

> > +
> >      gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
> >      gst_object_unref(decoder->appsrc);
> >      decoder->appsrc = NULL;
> > @@ -534,7 +542,7 @@ static bool launch_pipeline(SpiceGstDecoder
> > *decoder)
> >      }
> >      bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
> >      gst_bus_add_watch(bus, handle_pipeline_message, decoder);
> > -    gst_object_unref(bus);
> But the docs say that it is ok to unref the bus as the watch will take an additional
> reference on the bus.
>

Yes, it should be safe, but in this case the bus pointer is retained
to be able to remove the watch.
Yes, in theory we could store the bus pointer but still have the
gst_object_unref but nobody would prevent some other code to remove
the watch and make the pointer a dangling pointer. Keeping the
reference makes sure the pointer is not dangling.
Maybe a comment would help. I'll add one.

> Thanks,
> Vivek
> > +    decoder->bus = bus;
> >
> >      decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder-
> > >pipeline));
> >

Regards,
   Frediano