<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 2, 2017 at 12:36 AM, Aleksander Morgado <span dir="ltr"><<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Mar 1, 2017 at 3:59 AM, Ben Chan <<a href="mailto:benchan@chromium.org">benchan@chromium.org</a>> wrote:<br>
> find_device_by_physdev_uid() expects a non-NULL UID of the physical<br>
> device. However, mm_kernel_device_get_physdev_<wbr>uid() could potentially<br>
> return NULL on MMKernelDeviceUdev if find_physical_gudevdevice() in<br>
> mm-kernel-device-udev.c fails to find the physical device. When a NULL<br>
> physical device UID is passed to find_device_by_physdev_uid() and used<br>
> in g_hash_table_lookup(), it leads to a crash in g_str_hash(), which is<br>
> a bit obscure.<br>
><br>
> This patch updates kernel_device_get_physdev_uid(<wbr>) in<br>
> mm-kernel-device-udev.c to fall back to use the device sysfs if the<br>
> physical device sysfs isn't available, which is similar to how<br>
> kernel_device_get_physdev_uid(<wbr>) mm-kernel-device-generic.c is<br>
> implemented.<br>
> ---<br>
>  src/kerneldevice/mm-kernel-<wbr>device-udev.c | 29 ++++++++++++++++++++---------<br>
>  1 file changed, 20 insertions(+), 9 deletions(-)<br>
><br>
> diff --git a/src/kerneldevice/mm-kernel-<wbr>device-udev.c b/src/kerneldevice/mm-kernel-<wbr>device-udev.c<br>
> index 763ccf86..cd4f751f 100644<br>
> --- a/src/kerneldevice/mm-kernel-<wbr>device-udev.c<br>
> +++ b/src/kerneldevice/mm-kernel-<wbr>device-udev.c<br>
> @@ -358,19 +358,30 @@ kernel_device_get_physdev_uid (MMKernelDevice *_self)<br>
>      self = MM_KERNEL_DEVICE_UDEV (_self);<br>
><br>
>      /* Prefer the one coming in the properties, if any */<br>
> -    if (self->priv->properties)<br>
> -        uid = mm_kernel_event_properties_<wbr>get_uid (MM_KERNEL_DEVICE_UDEV (self)->priv->properties);<br>
> +    if (self->priv->properties) {<br>
> +        if ((uid = mm_kernel_event_properties_<wbr>get_uid (self->priv->properties)) != NULL)<br>
> +            return uid;<br>
> +    }<br>
><br>
> -    if (!uid) {<br>
> -        ensure_physdev (self);<br>
> -        if (!self->priv->physdev)<br>
> -            return NULL;<br>
> +    ensure_physdev (self);<br>
> +    if (self->priv->physdev) {<br>
> +        /* Try to load from properties set on the physical device */<br>
> +        if ((uid = g_udev_device_get_property (self->priv->physdev, "ID_MM_PHYSDEV_UID")) != NULL)<br>
> +            return uid;<br>
> +<br>
> +        /* Use physical device sysfs path, if any */<br>
> +        if ((uid = g_udev_device_get_sysfs_path (self->priv->physdev)) != NULL)<br>
> +            return uid;<br>
> +    }<br>
><br>
> -        uid = g_udev_device_get_property (self->priv->physdev, "ID_MM_PHYSDEV_UID");<br>
> -        if (!uid)<br>
> -            uid = g_udev_device_get_sysfs_path (self->priv->physdev);<br>
> +    if (self->priv->device) {<br>
<br>
</div></div>Is there any case where device may be NULL? I'd just "g_assert (device);" here.<br>
<span class=""><br></span></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +        /* If there is no physical device sysfs path, use the device sysfs itself */<br>
> +        if ((uid = g_udev_device_get_sysfs_path (self->priv->device)) != NULL)<br>
<br>
</span>And same here, maybe just "return g_udev_device_get_sysfs_path<br>
(self->priv->device):"?<br>
<br>
Every GUdevDevice should have a sysfs path, if I'm not missing anything.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> +            return uid;<br>
>      }<br>
><br>
> +    /* The caller of this function expects a non-NULL UID */<br>
> +    g_assert (uid != NULL);<br>
>      return uid;<br>
>  }<br>
><br>
> --<br>
> 2.12.0.rc1.440.g5b76565f74-<wbr>goog<br>
><br>
<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Aleksander<br>
<a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a><br>
</font></span></blockquote></div><br></div></div>