[PATCH] mm-base-manager: ref MMDevice before releasing port

[Dan Williams]

On Wed, 2018-02-21 at 13:49 -0800, Eric Caruso wrote:
> Releasing the port on the device looks benign but because it emits
> a signal, it could call device_context_port_released and unref the
> MMDevice in port_context_unref. This means the MMDevice might be
> disposed before we get to the g_object_ref and the subsequent call
> to g_hash_table_remove will try to hash a null string, which makes
> MM crash.

The refcounting looks a bit convoluted in the existing code, but the
patch LGTM for now.

Dan

> ---
>  src/mm-base-manager.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/src/mm-base-manager.c b/src/mm-base-manager.c
> index 4b92ab0b..7738ce0d 100644
> --- a/src/mm-base-manager.c
> +++ b/src/mm-base-manager.c
> @@ -220,28 +220,28 @@ device_removed (MMBaseManager  *self,
>          /* Handle tty/net/wdm port removal */
>          device = find_device_by_port (self, kernel_device);
>          if (device) {
> +            /* The callbacks triggered when the port is released or
> device support is
> +             * cancelled may end up unreffing the device or removing
> it from the HT, and
> +             * so in order to make sure the reference is still valid
> when we call
> +             * support_check_cancel() and g_hash_table_remove(), we
> hold a full reference
> +             * ourselves. */
> +            g_object_ref (device);
> +
>              mm_info ("(%s/%s): released by device '%s'", subsys,
> name, mm_device_get_uid (device));
>              mm_device_release_port (device, kernel_device);
>  
>              /* If port probe list gets empty, remove the device
> object iself */
>              if (!mm_device_peek_port_probe_list (device)) {
> -                /* The callback triggered when the device support is
> cancelled may end up
> -                 * removing the device from the HT, and that was the
> last full reference
> -                 * we kept. So, in order to make sure the reference
> is still valid after
> -                 * support_check_cancel(), we hold a full reference
> ourselves. */
>                  mm_dbg ("Removing empty device '%s'",
> mm_device_get_uid (device));
> -                g_object_ref (device);
> -                {
> -                    if
> (mm_plugin_manager_device_support_check_cancel (self->priv-
> >plugin_manager, device))
> -                        mm_dbg ("Device support check has been
> cancelled");
> -
> -                    /* The device may have already been removed from
> the tracking HT, we
> -                     * just try to remove it and if it fails, we
> ignore it */
> -                    mm_device_remove_modem (device);
> -                    g_hash_table_remove (self->priv->devices,
> mm_device_get_uid (device));
> -                }
> -                g_object_unref (device);
> +                if (mm_plugin_manager_device_support_check_cancel
> (self->priv->plugin_manager, device))
> +                    mm_dbg ("Device support check has been
> cancelled");
> +
> +                /* The device may have already been removed from the
> tracking HT, we
> +                 * just try to remove it and if it fails, we ignore
> it */
> +                mm_device_remove_modem (device);
> +                g_hash_table_remove (self->priv->devices,
> mm_device_get_uid (device));
>              }
> +            g_object_unref (device);
>          }
>  
>          return;