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

[Christophe Fergeau]

On Tue, Feb 28, 2017 at 09:34:48AM -0500, Frediano Ziglio wrote:
> 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."

Now that you ask, let's rework the commit log a bit :) But my initial
suggestion was just about the subject (aka shortlog).
"Do not assume the device passed as an argument to
spice_server_char_device_remove_interface() is the same as the current
Reds::vdagent instance. This commit adds a check that this is the case,
and returns early if not."

Christophe

> 
> (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;
> > >  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170228/22a581fe/attachment-0001.sig>