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

[Ben Chan]

On Thu, Mar 2, 2017 at 12:36 AM, Aleksander Morgado <
aleksander at aleksander.es> wrote:

> 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.
>
>
I saw a few self->priv->device check in mm-kernel-device-udev.c, so I was
wondering if there are cases where self->priv->device may be NULL. I'd keep
the check here for now as I haven't ruled out that possibility.


> > +        /* 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170302/75a2d6ed/attachment.html>