[Spice-devel] [PATCH] Avoid NULL-dereference if canvas_get_image errors out

[Christophe de Dinechin]

> On 3 Jul 2018, at 16:58, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Tue, Jul 03, 2018 at 04:31:10PM +0200, Christophe de Dinechin wrote:
>> I think we never had a discussion of what we really want in each case, and that’s causing the confusion.
>> 
>> First, a meta-rule. Like you, there is a lot in SPICE code I don’t
>> like. When in doubt, I try to use consistency as a guide. If the code
>> uses spice_critical in that file, my changes will use spice_critical.
>> That’s the case for canvas_base.c.
> 
> There are 2 occurrences of spice_critical in canvas_base.c, I'd push for
> changing these 2, and really discourage use of
> spice_critical/spice_warning in new code.

Again, agree to change globally. Disagree with you to connect the discussion with this patch, and disagree to create a local inconsistency for just a single line added.

> 
>> 
>> Then, on the specific point you raised. As I understand it,
>> spice_critical *may* abort, but it does not always abort, it depends
>> on the abort flag. Therefore, it is correct to free the resource in
>> case it returns. Do you disagree with any part of that rationale (i.e.
>> either disagree that spice_critical may return, or that we should free
>> if it does)?
> 
> spice_critical by default will abort(), it used to return in spice-gtk
> and abort() in spice-server, but this is no longer the case,
> spice_critical aborts in both cases. You could prevent spice_critical
> from aborting if you set SPICE_ABORT_LEVEL, but I don't think we should
> support someone reporting a bug after changing the default behaviour of
> spice_critical().

As long as there is a case (even if it’s not the default behaviour and infrequent) where spice_critical() returns, suggesting to *remove* the cleanup is incorrect. It’s not a matter of “not supporting”, it’s a matter of you requesting me to actually go ahead and change my code to break a case that is not broken in the patch as I submitted it.


> So for me spice_critical() aborts().

No, as stated above, and as stated by me earlier, spice_critical() *may* abort, but it may also *return*. That’s sufficient ground to leave the cleanup code in place.

> If that's what
> you want, I would not bother with the cleanups.

I want spice_ctitical because that’s what is being used in the file today (irrespective of what I may otherwise think about the incredibly confusing mess of SPICE and glib logging in general). And since spice_critical() may return, I need to leave the cleanup in place at the moment.


> If that's not what you
> want, I'd use g_critical(), g_warning(), spice_warning() instead, and do
> the cleanup.

I fail to see how g_critical is any different. It also may return or exit the program depending on some external environment. Only the default is different, but I do not write code to deal only with the default case, otherwise this patch would not even exist (canvas_get_image does not return NULL by default).


Thanks
Christophe

> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel