[PATCH v2 2/2] kernel-device: use device sysfs if physdev sysfs isn't available

[Aleksander Morgado]

On Wed, Mar 1, 2017 at 3:59 AM, 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 kernel_device_get_physdev_uid() in
> mm-kernel-device-udev.c to fall back to use the device sysfs if the
> physical device sysfs isn't available, which is similar to how
> kernel_device_get_physdev_uid() mm-kernel-device-generic.c is
> implemented.
> ---
>  src/kerneldevice/mm-kernel-device-udev.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/src/kerneldevice/mm-kernel-device-udev.c b/src/kerneldevice/mm-kernel-device-udev.c
> index 763ccf86..cd4f751f 100644
> --- a/src/kerneldevice/mm-kernel-device-udev.c
> +++ b/src/kerneldevice/mm-kernel-device-udev.c
> @@ -358,19 +358,30 @@ kernel_device_get_physdev_uid (MMKernelDevice *_self)
>      self = MM_KERNEL_DEVICE_UDEV (_self);
>
>      /* Prefer the one coming in the properties, if any */
> -    if (self->priv->properties)
> -        uid = mm_kernel_event_properties_get_uid (MM_KERNEL_DEVICE_UDEV (self)->priv->properties);
> +    if (self->priv->properties) {
> +        if ((uid = mm_kernel_event_properties_get_uid (self->priv->properties)) != NULL)
> +            return uid;
> +    }
>
> -    if (!uid) {
> -        ensure_physdev (self);
> -        if (!self->priv->physdev)
> -            return NULL;
> +    ensure_physdev (self);
> +    if (self->priv->physdev) {
> +        /* Try to load from properties set on the physical device */
> +        if ((uid = g_udev_device_get_property (self->priv->physdev, "ID_MM_PHYSDEV_UID")) != NULL)
> +            return uid;
> +
> +        /* Use physical device sysfs path, if any */
> +        if ((uid = g_udev_device_get_sysfs_path (self->priv->physdev)) != NULL)
> +            return uid;
> +    }
>
> -        uid = g_udev_device_get_property (self->priv->physdev, "ID_MM_PHYSDEV_UID");
> -        if (!uid)
> -            uid = g_udev_device_get_sysfs_path (self->priv->physdev);
> +    if (self->priv->device) {

Is there any case where device may be NULL? I'd just "g_assert (device);" here.

> +        /* If there is no physical device sysfs path, use the device sysfs itself */
> +        if ((uid = g_udev_device_get_sysfs_path (self->priv->device)) != NULL)

And same here, maybe just "return g_udev_device_get_sysfs_path
(self->priv->device):"?

Every GUdevDevice should have a sysfs path, if I'm not missing anything.

> +            return uid;
>      }
>
> +    /* The caller of this function expects a non-NULL UID */
> +    g_assert (uid != NULL);
>      return uid;
>  }
>
> --
> 2.12.0.rc1.440.g5b76565f74-goog
>



-- 
Aleksander
https://aleksander.es