<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 24, 2017 at 10:55 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Thu, Feb 23, 2017 at 8:45 PM, 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 find_device_by_physdev_uid() to handle the case when<br>
> mm_kernel_device_get_physdev_<wbr>uid() fails to return a valid UID, and also<br>
> moves the assertion on non-NULL UID to find_device_by_physdev_uid() in<br>
> order to ensure find_device_by_physdev_uid() is properly used by future<br>
> callers.<br>
<br>
</span>I'm not sure I agree with this patch. We do want to enforce non-NULL<br>
UIDs, as that UID (it previously was the "physdev sysfs path" but<br>
doesn't have to now) is used to group ports of the same device<br>
together. If the device exposes a single port and has no parent device<br>
per se, the sysfs path of the port itself may be used as UID, as<br>
there's no grouping to be done.<br>
<br>
What we should make sure is that mm_kernel_device_get_physdev_<wbr>uid()<br>
never returns NULL, and so the g_assert() is correctly in place.<br>
<br>
What do you think about these comments?<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br></div></div></blockquote><div><br></div><div>If that's the expectation, the g_assert() should be placed inside mm_kernel_device_get_physdev_<wbr>uid () instead of the call sites of mm_kernel_device_get_physdev_<wbr>uid (), which is easier to omit. </div><div><br></div><div>I can update the patch to move the g_assert () and modify the code to provide a fallback value of physdev uid to ensure it's never NULL. WDYT?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">
> ---<br>
>  src/mm-base-manager.c | 10 ++++++++--<br>
>  1 file changed, 8 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/mm-base-manager.c b/src/mm-base-manager.c<br>
> index 66096130..d5049b12 100644<br>
> --- a/src/mm-base-manager.c<br>
> +++ b/src/mm-base-manager.c<br>
> @@ -127,6 +127,8 @@ static MMDevice *<br>
>  find_device_by_physdev_uid (MMBaseManager *self,<br>
>                              const gchar   *physdev_uid)<br>
>  {<br>
> +    g_assert (physdev_uid != NULL);<br>
> +<br>
>      return g_hash_table_lookup (self->priv->devices, physdev_uid);<br>
>  }<br>
><br>
> @@ -134,7 +136,12 @@ static MMDevice *<br>
>  find_device_by_kernel_device (MMBaseManager  *manager,<br>
>                                MMKernelDevice *kernel_device)<br>
>  {<br>
> -    return find_device_by_physdev_uid (manager, mm_kernel_device_get_physdev_<wbr>uid (kernel_device));<br>
> +    const gchar *physdev_uid;<br>
> +<br>
> +    physdev_uid = mm_kernel_device_get_physdev_<wbr>uid (kernel_device);<br>
> +    g_return_val_if_fail (physdev_uid != NULL, NULL);<br>
> +<br>
> +    return find_device_by_physdev_uid (manager, physdev_uid);<br>
>  }<br>
><br>
>  /*****************************<wbr>******************************<wbr>******************/<br>
> @@ -277,7 +284,6 @@ device_added (MMBaseManager  *manager,<br>
>      /* Get the port's physical device's uid. All ports of the same physical<br>
>       * device will share the same uid. */<br>
>      physdev_uid = mm_kernel_device_get_physdev_<wbr>uid (port);<br>
> -    g_assert (physdev_uid);<br>
><br>
>      /* See if we already created an object to handle ports in this device */<br>
>      device = find_device_by_physdev_uid (manager, physdev_uid);<br>
> --<br>
> 2.11.0.483.g087da7b7c-goog<br>
><br>
<br>
<br>
<br>
</div></div><span class="gmail-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>