[spice-server] reds: Free device chain in spice_server_destroy to avoid leaks

Submitted by Christophe Fergeau on July 13, 2018, 11:52 a.m.

Details

Message ID 20180713115257.GF5388@natto.ory.fergeau.eu
State New
Headers show
Series "reds: Free device chain in spice_server_destroy to avoid leaks" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe Fergeau July 13, 2018, 11:52 a.m.
On Wed, Jul 11, 2018 at 05:30:25PM +0100, Frediano Ziglio wrote:
> Leak detectors did not manage to find leaks, possibly as double list
> have all elements likely with a pointer to them.
> The reference from the agent is necessary for inserting it into
> the list.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/reds.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index f1e34529a..85043a88d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3132,6 +3132,7 @@ static int spice_server_char_device_add_interface(SpiceServer *reds,
>              return -1;
>          }
>          dev_state = attach_to_red_agent(reds, char_device);
> +        g_object_ref(dev_state);

This g_object_ref() looks a bit out of place to be honest. It's there
so that attach_to_red_agent() behaves similarly to the
xxx_device_connect() calls in that we want to own a reference to
dev_state. Imo it would make more sense to move the g_object_ref()
inside attach_to_red_agent() as it's an implementation detail of that
method that we return the same object rather than creating a new one.
Actually, that whole thing reds->char_devices which somehow owns a ref,
but that ref is released in
spicevmc_device_disconnect/smartcard_device_disconnect even if their
implementations never took/owned that ref is quite convoluted. The
attache patch cleans up things imo, but I've only compile-tested it, so
it's likely broken..


>      }
>  #ifdef USE_SMARTCARD
>      else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
> @@ -3682,6 +3683,19 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *reds)
>      }
>      reds_cleanup_net(reds);
>      g_clear_object(&reds->agent_dev);
> +
> +    // NOTE: don't replace with g_list_free_full as this function that passed callback
> +    // don't change the list while g_object_unref in this case will change it.
> +    RedCharDevice *dev;
> +    GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> +        g_object_unref(dev);
> +    }
> +    g_list_free(reds->char_devices);
> +    reds->char_devices = NULL;
> +
> +    g_list_free(reds->channels);
> +    reds->channels = NULL;

If reds->channels is not empty, this means we did not call
reds_unregister_channel, and so we'll actually be leaking what is in the
list. At the moment, it's odd that SpiceServer owns a ref to the input
and main channels, but not to the other channels in reds->channels. It
seems it should be possible to add reds_{register,unregister}_channel()
to take ownership of the channels so that reds->channels also owns a
ref.

Christophe

Patch hide | download patch | download mbox

diff --git a/server/char-device.c b/server/char-device.c
index ae538fab1..94bbe15c0 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -694,12 +694,6 @@  void red_char_device_reset_dev_instance(RedCharDevice *dev,
     g_object_notify(G_OBJECT(dev), "sin");
 }
 
-void red_char_device_destroy(RedCharDevice *char_dev)
-{
-    g_return_if_fail(RED_IS_CHAR_DEVICE(char_dev));
-    g_object_unref(char_dev);
-}
-
 static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
                                                        int do_flow_control,
                                                        uint32_t max_send_queue_size,
diff --git a/server/reds.c b/server/reds.c
index 85043a88d..e44fac264 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3025,7 +3025,7 @@  static RedCharDevice *attach_to_red_agent(RedsState *reds, SpiceCharDeviceInstan
     }
 
     if (!reds_main_channel_connected(reds)) {
-        return RED_CHAR_DEVICE(dev);
+        return g_object_ref(RED_CHAR_DEVICE(dev));
     }
 
     dev->priv->read_filter.discard_all = FALSE;
@@ -3073,7 +3073,7 @@  static RedCharDevice *attach_to_red_agent(RedsState *reds, SpiceCharDeviceInstan
         main_channel_push_agent_connected(reds->main_channel);
     }
 
-    return RED_CHAR_DEVICE(dev);
+    return g_object_ref(RED_CHAR_DEVICE(dev));
 }
 
 SPICE_GNUC_VISIBLE void spice_server_char_device_wakeup(SpiceCharDeviceInstance* sin)
@@ -3106,16 +3106,16 @@  SPICE_GNUC_VISIBLE const char** spice_server_char_device_recognized_subtypes(voi
 
 static void reds_add_char_device(RedsState *reds, RedCharDevice *dev)
 {
-    reds->char_devices = g_list_append(reds->char_devices, dev);
+    reds->char_devices = g_list_append(reds->char_devices, g_object_ref(dev));
 }
 
-static void reds_on_char_device_destroy(RedsState *reds,
-                                        RedCharDevice *dev)
+static void reds_remove_char_device(RedsState *reds, RedCharDevice *dev)
 {
     g_return_if_fail(reds != NULL);
     g_warn_if_fail(g_list_find(reds->char_devices, dev) != NULL);
 
     reds->char_devices = g_list_remove(reds->char_devices, dev);
+    g_object_unref(dev);
 }
 
 static int spice_server_char_device_add_interface(SpiceServer *reds,
@@ -3132,7 +3132,6 @@  static int spice_server_char_device_add_interface(SpiceServer *reds,
             return -1;
         }
         dev_state = attach_to_red_agent(reds, char_device);
-        g_object_ref(dev_state);
     }
 #ifdef USE_SMARTCARD
     else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
@@ -3160,15 +3159,13 @@  static int spice_server_char_device_add_interface(SpiceServer *reds,
          * just a sanity check to ensure that assumption is correct */
         spice_assert(dev_state == char_device->st);
 
-        g_object_weak_ref(G_OBJECT(dev_state),
-                          (GWeakNotify)reds_on_char_device_destroy,
-                          reds);
         /* setting the char_device state to "started" for backward compatibily with
          * qemu releases that don't call spice api for start/stop (not implemented yet) */
         if (reds->vm_running) {
             red_char_device_start(dev_state);
         }
         reds_add_char_device(reds, dev_state);
+        g_object_unref(dev_state);
     } else {
         spice_warning("failed to create device state for %s", char_device->subtype);
         return -1;
@@ -3188,19 +3185,14 @@  static int spice_server_char_device_remove_interface(RedsState *reds, SpiceBaseI
             reds_agent_remove(reds);
             red_char_device_reset_dev_instance(RED_CHAR_DEVICE(reds->agent_dev), NULL);
         }
-    }
+    } else if (strcmp(char_device->subtype, SUBTYPE_USBREDIR) != 0 &&
 #ifdef USE_SMARTCARD
-    else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
-        smartcard_device_disconnect(char_device);
-    }
+               strcmp(char_device->subtype, SUBTYPE_SMARTCARD) != 0 &&
 #endif
-    else if (strcmp(char_device->subtype, SUBTYPE_USBREDIR) == 0 ||
-             strcmp(char_device->subtype, SUBTYPE_PORT) == 0) {
-        spicevmc_device_disconnect(reds, char_device);
-    } else {
-        spice_warning("failed to remove char device %s", char_device->subtype);
+               strcmp(char_device->subtype, SUBTYPE_PORT) != 0) {
+        g_warning("failed to remove char device %s", char_device->subtype);
     }
-
+    reds_remove_char_device(reds, RED_CHAR_DEVICE(char_device->st));
     char_device->st = NULL;
     return 0;
 }
@@ -3684,13 +3676,7 @@  SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *reds)
     reds_cleanup_net(reds);
     g_clear_object(&reds->agent_dev);
 
-    // NOTE: don't replace with g_list_free_full as this function that passed callback
-    // don't change the list while g_object_unref in this case will change it.
-    RedCharDevice *dev;
-    GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
-        g_object_unref(dev);
-    }
-    g_list_free(reds->char_devices);
+    g_list_free_full(reds->char_devices, g_object_unref);
     reds->char_devices = NULL;
 
     g_list_free(reds->channels);
diff --git a/server/smartcard.c b/server/smartcard.c
index 2cb68e066..b7e111ecf 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -284,13 +284,6 @@  static RedCharDeviceSmartcard *smartcard_device_new(RedsState *reds, SpiceCharDe
     return RED_CHAR_DEVICE_SMARTCARD(char_dev);
 }
 
-void smartcard_device_disconnect(SpiceCharDeviceInstance *char_device)
-{
-    g_return_if_fail(RED_IS_CHAR_DEVICE_SMARTCARD(char_device->st));
-
-    g_object_unref(char_device->st);
-}
-
 RedCharDevice *smartcard_device_connect(RedsState *reds, SpiceCharDeviceInstance *char_device)
 {
     RedCharDeviceSmartcard *dev;
diff --git a/server/smartcard.h b/server/smartcard.h
index f0b4fa440..7ee09fa5d 100644
--- a/server/smartcard.h
+++ b/server/smartcard.h
@@ -45,7 +45,6 @@  struct RedCharDeviceSmartcardClass
  * connect to smartcard interface, used by smartcard channel
  */
 RedCharDevice *smartcard_device_connect(RedsState *reds, SpiceCharDeviceInstance *char_device);
-void smartcard_device_disconnect(SpiceCharDeviceInstance *char_device);
 void smartcard_channel_write_to_reader(RedCharDeviceWriteBuffer *write_buf);
 SpiceCharDeviceInstance* smartcard_readers_get(uint32_t reader_id);
 SpiceCharDeviceInstance *smartcard_readers_get_unattached(void);
diff --git a/server/spicevmc.c b/server/spicevmc.c
index c2de5037f..abd5d91b6 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -824,13 +824,6 @@  RedCharDevice *spicevmc_device_connect(RedsState *reds,
     return dev;
 }
 
-/* Must be called from RedClient handling thread. */
-void spicevmc_device_disconnect(RedsState *reds, SpiceCharDeviceInstance *sin)
-{
-    g_object_unref(RED_CHAR_DEVICE(sin->st));
-    sin->st = NULL;
-}
-
 static void spicevmc_port_event(RedCharDevice *char_dev, uint8_t event)
 {
     RedVmcChannel *channel;