> > 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