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

[Aleksander Morgado]

On 21/02/18 22:49, 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.

Yeah, looks like PORT_REMOVED signal handler may end up calling device_support_check_ready() and that removes the device also from the HT. I'm going to push a follow up commit adding the explicit extra indentation between the ref/unref to make it clear that this is just to keep a reference valid for that block of logic.

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


-- 
Aleksander
https://aleksander.es