Re: [PATCH] device: increase object ref count before invoking g_idle_add

[Thomas Haller <thaller redhat com>]

On Wed, 2016-05-04 at 17:27 +0800, Shih-Yuan Lee (FourDollars) wrote:
> ---
>  src/devices/nm-device.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
> index ad6f835..77f8874 100644
> --- a/src/devices/nm-device.c
> +++ b/src/devices/nm-device.c
> @@ -1990,8 +1990,8 @@ realize_start_setup (NMDevice *self, const
> NMPlatformLink *plink)
>       }
>  
>       /* trigger initial ip config change to initialize ip-config
> */
> -     priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, self);
> -     priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, self);
> +     priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, g_object_ref (self));
> +     priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, g_object_ref (self));
>  
>       nm_device_update_hw_address (self);
>       nm_device_update_initial_hw_address (self);
> @@ -6982,6 +6982,7 @@ queued_ip4_config_change_clear (NMDevice *self)
>               _LOGD (LOGD_DEVICE, "clearing queued IP4 config
> change");
>               g_source_remove (priv->queued_ip4_config_id);
>               priv->queued_ip4_config_id = 0;
> +             g_object_unref (self);
>       }
>  }
>  
> @@ -6994,6 +6995,7 @@ queued_ip6_config_change_clear (NMDevice *self)
>               _LOGD (LOGD_DEVICE, "clearing queued IP6 config
> change");
>               g_source_remove (priv->queued_ip6_config_id);
>               priv->queued_ip6_config_id = 0;
> +             g_object_unref (self);
>       }
>  }
>  
> @@ -8942,7 +8944,11 @@ update_ip4_config (NMDevice *self, gboolean
> initial)
>       if (activation_source_is_scheduled (self,
>                                           activate_stage5_ip4_conf
> ig_commit,
>                                           AF_INET)) {
> -             priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, self);
> +             if (priv->queued_ip4_config_id) {
> +                     g_source_remove (priv-
> > queued_ip4_config_id);
> +                     g_object_unref (self);
> +             }
> +             priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, g_object_ref (self));
>               _LOGT (LOGD_DEVICE, "IP4 update was postponed");
>               return;
>       }
> @@ -9031,7 +9037,11 @@ update_ip6_config (NMDevice *self, gboolean
> initial)
>       if (activation_source_is_scheduled (self,
>                                           activate_stage5_ip6_conf
> ig_commit,
>                                           AF_INET6)) {
> -             priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, self);
> +             if (priv->queued_ip6_config_id) {
> +                     g_source_remove (priv-
> > queued_ip6_config_id);
> +                     g_object_unref (self);
> +             }
> +             priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, g_object_ref (self));
>               _LOGT (LOGD_DEVICE, "IP6 update was postponed");
>               return;
>       }
> @@ -9109,8 +9119,8 @@ queued_ip4_config_change (gpointer user_data)
>               return TRUE;
>  
>       priv->queued_ip4_config_id = 0;
> -     g_object_ref (self);
>       update_ip4_config (self, FALSE);
> +
>       g_object_unref (self);
>  
>       set_unmanaged_external_down (self, TRUE);
> @@ -9131,7 +9141,6 @@ queued_ip6_config_change (gpointer user_data)
>               return TRUE;
>  
>       priv->queued_ip6_config_id = 0;
> -     g_object_ref (self);
>       update_ip6_config (self, FALSE);
>  
>       if (   nm_platform_link_get (NM_PLATFORM_GET, priv->ifindex)
> @@ -9194,7 +9203,7 @@ device_ipx_changed (NMPlatform *platform,
>       case NMP_OBJECT_TYPE_IP4_ADDRESS:
>       case NMP_OBJECT_TYPE_IP4_ROUTE:
>               if (!priv->queued_ip4_config_id) {
> -                     priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, self);
> +                     priv->queued_ip4_config_id = g_idle_add
> (queued_ip4_config_change, g_object_ref (self));
>                       _LOGD (LOGD_DEVICE, "queued IP4 config
> change");
>               }
>               break;
> @@ -9211,7 +9220,7 @@ device_ipx_changed (NMPlatform *platform,
>               /* fallthrough */
>       case NMP_OBJECT_TYPE_IP6_ROUTE:
>               if (!priv->queued_ip6_config_id) {
> -                     priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, self);
> +                     priv->queued_ip6_config_id = g_idle_add
> (queued_ip6_config_change, g_object_ref (self));
>                       _LOGD (LOGD_DEVICE, "queued IP6 config
> change");
>               }
>               break;


Hi,


For any pending queued_ip4_config_change(), we track the gsource ID
in queued_ip4_config_id.
Thus, the right fix is not to take an additional reference for @self,
but ensuring that the queued_ip4_config_id is cleared (g_clear_source)
before the device object gets destroyed.



From looking at the code, it is however unclear how this crash could
possibly happen:

  dispose()
  -> _cleanup_generic_pre()
    -> _cleanup_ip4_pre()
      -> queued_ip4_config_change_clear().

Maybe the right fix for this is 
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0175056a6d70bafdaf1042eb8f5e1ef57484a3f2


Thomas

Attachment: signature.asc
Description: This is a digitally signed message part