[PATCH 2/3] base-manager: handle NULL physical device more gracefully

[Aleksander Morgado]

On Thu, Feb 23, 2017 at 8:45 PM, Ben Chan <benchan at chromium.org> wrote:
> find_device_by_physdev_uid() expects a non-NULL UID of the physical
> device. However, mm_kernel_device_get_physdev_uid() could potentially
> return NULL on MMKernelDeviceUdev if find_physical_gudevdevice() in
> mm-kernel-device-udev.c fails to find the physical device. When a NULL
> physical device UID is passed to find_device_by_physdev_uid() and used
> in g_hash_table_lookup(), it leads to a crash in g_str_hash(), which is
> a bit obscure.
>
> This patch updates find_device_by_physdev_uid() to handle the case when
> mm_kernel_device_get_physdev_uid() fails to return a valid UID, and also
> moves the assertion on non-NULL UID to find_device_by_physdev_uid() in
> order to ensure find_device_by_physdev_uid() is properly used by future
> callers.

I'm not sure I agree with this patch. We do want to enforce non-NULL
UIDs, as that UID (it previously was the "physdev sysfs path" but
doesn't have to now) is used to group ports of the same device
together. If the device exposes a single port and has no parent device
per se, the sysfs path of the port itself may be used as UID, as
there's no grouping to be done.

What we should make sure is that mm_kernel_device_get_physdev_uid()
never returns NULL, and so the g_assert() is correctly in place.

What do you think about these comments?

> ---
>  src/mm-base-manager.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/mm-base-manager.c b/src/mm-base-manager.c
> index 66096130..d5049b12 100644
> --- a/src/mm-base-manager.c
> +++ b/src/mm-base-manager.c
> @@ -127,6 +127,8 @@ static MMDevice *
>  find_device_by_physdev_uid (MMBaseManager *self,
>                              const gchar   *physdev_uid)
>  {
> +    g_assert (physdev_uid != NULL);
> +
>      return g_hash_table_lookup (self->priv->devices, physdev_uid);
>  }
>
> @@ -134,7 +136,12 @@ static MMDevice *
>  find_device_by_kernel_device (MMBaseManager  *manager,
>                                MMKernelDevice *kernel_device)
>  {
> -    return find_device_by_physdev_uid (manager, mm_kernel_device_get_physdev_uid (kernel_device));
> +    const gchar *physdev_uid;
> +
> +    physdev_uid = mm_kernel_device_get_physdev_uid (kernel_device);
> +    g_return_val_if_fail (physdev_uid != NULL, NULL);
> +
> +    return find_device_by_physdev_uid (manager, physdev_uid);
>  }
>
>  /*****************************************************************************/
> @@ -277,7 +284,6 @@ device_added (MMBaseManager  *manager,
>      /* Get the port's physical device's uid. All ports of the same physical
>       * device will share the same uid. */
>      physdev_uid = mm_kernel_device_get_physdev_uid (port);
> -    g_assert (physdev_uid);
>
>      /* See if we already created an object to handle ports in this device */
>      device = find_device_by_physdev_uid (manager, physdev_uid);
> --
> 2.11.0.483.g087da7b7c-goog
>



-- 
Aleksander
https://aleksander.es