[Spice-devel] [spice-server] Add some NULL checks to spice_server_remove_interface()

[Frediano Ziglio]

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

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

or C99:

    g_return_val_if_fail(sin != NULL, -2);

    const SpiceBaseInterface *interface = sin->sif;

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

Beside these small comments patch looks fine.

Frediano