[Spice-devel] [PATCH spice-server] reds: Free device chain in spice_server_destroy to avoid leaks

[Frediano Ziglio]

> 
> On 11/07/18 13:30, Frediano Ziglio wrote:
> > Leak detectors did not manage to find leaks, possibly as double list
> > have all elements likely with a pointer to them.
> > The reference from the agent is necessary for inserting it into
> > the list.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/reds.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index f1e34529a..85043a88d 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3132,6 +3132,7 @@ static int
> > spice_server_char_device_add_interface(SpiceServer *reds,
> >              return -1;
> >          }
> >          dev_state = attach_to_red_agent(reds, char_device);
> > +        g_object_ref(dev_state);
> >      }
> >  #ifdef USE_SMARTCARD
> >      else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
> > @@ -3682,6 +3683,19 @@ SPICE_GNUC_VISIBLE void
> > spice_server_destroy(SpiceServer *reds)
> >      }
> >      reds_cleanup_net(reds);
> >      g_clear_object(&reds->agent_dev);
> > +
> > +    // NOTE: don't replace with g_list_free_full as this function that
> > passed callback
> > +    // don't change the list while g_object_unref in this case will change
> > it.
> > +    RedCharDevice *dev;
> > +    GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> > +        g_object_unref(dev);
> > +    }
> > +    g_list_free(reds->char_devices);
> > +    reds->char_devices = NULL;
> > +
> > +    g_list_free(reds->channels);
> > +    reds->channels = NULL;
> > +
> 
> g_list_free() + NULL can be replaced with g_clear_pointer()
> 

I did a mistake and committed this patch instead of the
"red-stream-device: Fix leaks in dispose and finalize chaining parent"
(which was acked by Christophe Fergeau).
Now with the 2 patches on master Valgrind is happy and CI is working.

Not exactly sure if this patch is 100% correct, I think so but is
weird that agent_dev can be inside the device list or not based on Qemu
parameters (without Qemu base on "vdagent" interface registration).
Got the feeling that something is potentially broken in all these
references. In theory for instance the "vdagent" interface can be
unregistered leaving with the agent_dev still in the list but a
future potential addition of another "vdagent" interface would add
the agent_dev again to the list. Using Qemu I don't think this is
possible (interfaces are added and never removed).
Note that devices are removed from the list only if the object reference
got 0 but in case of agent_dev when the interface is removed the agent_dev
will still be valid and won't be removed from the list automatically.
Maybe we should not remove from the list automatically?

Frediano