Re: [spice-server] Add some NULL checks to spice_server_remove_interface()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Mon, Jan 30, 2017 at 01:26:01PM -0500, Frediano Ziglio wrote:
> > > 
> > > Currently, calling spice_server_remove_interface() twice in a row with
> > > the same SPICE_INTERFACE_CHAR_DEVICE is going to cause a crash when
> > > calling red_char_device_get_server(char_device->st); because
> > > char_device->st will have been set to NULL by the first call.
> > > 
> > > This commit adds a few sanity checks before trying to use the various
> > > 'st' members of the interfaces.
> > > 
> > > This should avoid the crash described in
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1411194 even though it's not
> > > clear how we got in that situation.
> > 
> > Yes, there is no description on what he was doing.
> > Maybe migration with device closed ?
> 
> As you pointed out in the bug, there are two '-chardev
> spicevmc,id=spicechannel1,name=vdagent ' instances on the command line.
> Might be related even though I did not trigger the crash/warnings in
> that setup either.
> 

depends on some race conditions.

> > 
> > > ---
> > >  server/reds.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 29485a8..90d126d 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -3307,8 +3307,11 @@ SPICE_GNUC_VISIBLE int
> > > spice_server_remove_interface(SpiceBaseInstance *sin)
> > >      RedsState *reds;
> > >      const SpiceBaseInterface *interface = sin->sif;
> > >  
> > > +    g_return_val_if_fail(sin != NULL, -2);
> > > +
> > 
> > sin is already used, should be
> > 
> >     const SpiceBaseInterface *interface;
> >  
> >     g_return_val_if_fail(sin != NULL, -2);
> > 
> >     interface = sin->sif;
> 
> Ah right, I changed it to this version. Just added this for extra
> safety, I don't think this is what is happening in this case (and
> arguably it's odd to do the check there and not in _add_interface, and
> most public entry points).
> 
> 
> 
> > >      if (strcmp(interface->type, SPICE_INTERFACE_TABLET) == 0) {
> > >          SpiceTabletInstance *tablet = SPICE_CONTAINEROF(sin,
> > >          SpiceTabletInstance, base);
> > > +        g_return_val_if_fail(tablet->st != NULL, -2);
> > >          reds = spice_tablet_state_get_server(tablet->st);
> > >          spice_info("remove SPICE_INTERFACE_TABLET");
> > >          inputs_channel_detach_tablet(reds->inputs_channel, tablet);
> > > @@ -3321,12 +3324,14 @@ SPICE_GNUC_VISIBLE int
> > > spice_server_remove_interface(SpiceBaseInstance *sin)
> > >          snd_detach_record(SPICE_CONTAINEROF(sin, SpiceRecordInstance,
> > >          base));
> > >      } else if (strcmp(interface->type, SPICE_INTERFACE_CHAR_DEVICE) ==
> > >      0) {
> > >          SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin,
> > >          SpiceCharDeviceInstance, base);
> > > +        g_return_val_if_fail(char_device->st != NULL, -2);
> > >          reds = red_char_device_get_server(char_device->st);
> > >          spice_server_char_device_remove_interface(reds, sin);
> > >      } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> > >          QXLInstance *qxl;
> > >  
> > >          qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> > > +        g_return_val_if_fail(qxl->st != NULL, -2);
> > >          reds = red_qxl_get_server(qxl->st);
> > >          reds->qxl_instances = g_list_remove(reds->qxl_instances, qxl);
> > >          red_qxl_destroy(qxl);
> > 
> > Why -2 ?
> > 
> Did not know whether to reuse -1, or to return a different error code.
> I chose to return -2 as a different code, I don't mind reusing -1 if
> that's preferred.
> 
> Christophe
> 

I looked at server/spice-server.h and there's no documentation, just the function
returns int and from code 0 for success and once -1 for error.
So even 1 would be fine... no idea, we should add some documentation probably.
Qemu seems to ignore the return beside when create displays.
I would just return -1 as many C/POSIX functions returns -1 for error.
But I think we are free.. perhaps -EINVAL or -ENODEV ? (in this case -1 would
mean -EPERM, if we define like that).

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]