Re: [Spice-devel] [spice-gtk v2] usb-device-widget: Fix crash on no USB devices
>
> 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 <907246...@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
> >>> ---
> >>> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2] usb-device-widget: Fix crash on no USB devices
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 <907246...@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
>>> ---
>>> 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
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2] usb-device-widget: Fix crash on no USB devices
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 <907246...@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
> > ---
> > 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).
Cheers,
toso
>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etru...@redhat.com
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2] usb-device-widget: Fix crash on no USB devices
On 13/04/18 05:50, Victor Toso wrote:
> From: goldengdeng <907246...@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
> ---
> 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)? Would be interesting to see the
backtrace for this supposed crash, because this patch does not seem
correct to me.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
