[Spice-devel] [spice-gtk v2] usb-device-widget: Fix crash on no USB devices

[Frediano Ziglio]

> 
> On 13/04/18 18:20, Victor Toso wrote:
> > Hi,
> > 
> > On Fri, Apr 13, 2018 at 03:14:56PM -0300, Eduardo Lima (Etrunko) wrote:
> >> On 13/04/18 05:50, Victor Toso wrote:
> >>> From: goldengdeng <907246009 at qq.com>
> >>>
> >>> The spice_usb_device_manager_get_devices() is only checking for NULL
> >>> while the program can crash when no USB devices are available.
> >>>
> >>> Signed-off-by: Victor Toso <victortoso at redhat.com>
> >>> ---
> >>>  src/usb-device-widget.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
> >>> index a3c0910..1be80ae 100644
> >>> --- a/src/usb-device-widget.c
> >>> +++ b/src/usb-device-widget.c
> >>> @@ -218,8 +218,9 @@ static void
> >>> spice_usb_device_widget_constructed(GObject *gobject)
> >>>                       G_CALLBACK(device_error_cb), self);
> >>>  
> >>>      devices = spice_usb_device_manager_get_devices(priv->manager);
> >>> -    if (!devices)
> >>> +    if (devices == NULL || devices->len == 0) {
> >>>          goto end;
> >>> +    }
> >>
> >> Does it mean that the crash is happening on g_ptr_array_unref() call
> >> (which happens after the loop below)?
> > 
> > Even if we call g_ptr_array_unref() with NULL, it just log some
> > criticals.
> > 
> >> Would be interesting to see the backtrace for this supposed
> >> crash, because this patch does not seem correct to me.
> > 
> > Yeah, I agree. I've asked in the original email too although the
> > change itself is not complex I don't want to dive into 'where
> > could it crash' but for sure we should check if devices is NULL
> > (the original patch removed that).
> > 
> 
> I agree with the check for NULL, but the new conditional for
> devices->len is already done in the loop, thus the reason I asked for
> the trace.
> 
> Regards, Eduardo
> 

Agreed, looks like this patch is just adding a leak if the returned
array has 0 elements.
>From the last patch from Victor looks like this patch is silently
nacked.

Frediano