[Spice-devel] [PATCH spice-server] reds: Do not free wrong agent device

[Frediano Ziglio]

Just to be clear:

"reds: Check that we do not free the wrong agent device

Do not assume the device passed is the one currently working
but check for it and refuse to free the current one avoiding
possible double free of the device."

(was not clear to me if you meant the subject or message)

Frediano

> From: "Christophe Fergeau" <cfergeau at redhat.com>
> 
> I think I'd adjust the shortlog a little bit "Check that we do not free
> ...", as I initially thought this was fixing an actual bug rather than
> being a robustness improvement.
> Hopefully we don't hit this situation, as this would be fairly bad ;)
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 
> 
> 
> On Tue, Feb 28, 2017 at 10:26:10AM +0000, Frediano Ziglio wrote:
> > Do not assume the device passed is the one currently working
> > but check for it and refuse to free the current one avoiding
> > possible double free of the device.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/reds.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 39a7a31..88ee7d1 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3201,13 +3201,14 @@ static int
> > spice_server_char_device_add_interface(SpiceServer *reds,
> >      return 0;
> >  }
> >  
> > -static void spice_server_char_device_remove_interface(RedsState *reds,
> > SpiceBaseInstance *sin)
> > +static int spice_server_char_device_remove_interface(RedsState *reds,
> > SpiceBaseInstance *sin)
> >  {
> >      SpiceCharDeviceInstance* char_device =
> >              SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
> >  
> >      spice_info("remove CHAR_DEVICE %s", char_device->subtype);
> >      if (strcmp(char_device->subtype, SUBTYPE_VDAGENT) == 0) {
> > +        g_return_val_if_fail(char_device == reds->vdagent, -1);
> >          if (reds->vdagent) {
> >              reds_agent_remove(reds);
> >              red_char_device_reset_dev_instance(RED_CHAR_DEVICE(reds->agent_dev),
> >              NULL);
> > @@ -3226,6 +3227,7 @@ static void
> > spice_server_char_device_remove_interface(RedsState *reds, SpiceBase
> >      }
> >  
> >      char_device->st = NULL;
> > +    return 0;
> >  }
> >  
> >  SPICE_GNUC_VISIBLE int spice_server_add_interface(SpiceServer *reds,
> > @@ -3360,7 +3362,7 @@ SPICE_GNUC_VISIBLE int
> > spice_server_remove_interface(SpiceBaseInstance *sin)
> >          SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin,
> >          SpiceCharDeviceInstance, base);
> >          g_return_val_if_fail(char_device->st != NULL, -1);
> >          reds = red_char_device_get_server(char_device->st);
> > -        spice_server_char_device_remove_interface(reds, sin);
> > +        return spice_server_char_device_remove_interface(reds, sin);
> >      } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> >          QXLInstance *qxl;
> >