Re: Missing qapi_free_Type in error case for qapi generated code?

[Markus Armbruster <armbru@redhat.com>]

Christophe de Dinechin <dinechin@redhat.com> writes:

> On 2020-07-29 at 10:34 CEST, Markus Armbruster wrote...
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 7/28/20 10:26 AM, Christophe de Dinechin wrote:
>>>> The qapi generated code for qmp_marshal_query_spice seems to be missing a
>>>> resource deallocation for "retval". For example, for SpiceInfo:
>>>>
>>>
>>>>      retval = qmp_query_spice(&err);
>>>>      error_propagate(errp, err);
>>>>      if (err) {
>>>> /* retval not freed here */
>>>
>>> Because it should be NULL here.  Returning an error AND an object is
>>> frowned on.
>>
>> It's forbidden, actually.  The QMP handler must either succeed and
>> return a value, or fail cleanly.
>
> OK. Then I guess Eric's suggestion to add an assert is the correct
> approach, with the caveat you identified.

I'm not sure it's worth the trouble, but I'm of course happy to review a
patch.

>> Since it has to return a value even when it fails, it returns an error
>> value then.  "Cleanly" means the error value does not require cleanup.
>>
>> The generated marshalling function relies on this: it *ignores* the
>> error value.
>>
>>>> /* Missing: qapi_free_SpiceInfo(retval); */
>>>>          goto out;
>>>>      }
>>>>
>>>>      qmp_marshal_output_SpiceInfo(retval, ret, errp);
>>>
>>> And here, retval was non-NULL, but is cleaned as a side-effect of
>>> qmp_marshal_output_SpiceInfo.
>>>
>>>>
>>>> out:
>>>
>>> So no matter how you get to the label, retval is no longer valid
>>> memory that can be leaked.
>>>
>>>>      visit_free(v);
>>>>      v = qapi_dealloc_visitor_new();
>>>>      visit_start_struct(v, NULL, NULL, 0, NULL);
>>>>      visit_end_struct(v, NULL);
>>>>      visit_free(v);
>>>> }
>>>> #endif /* defined(CONFIG_SPICE) */
>>>>
>>>> Questions:
>>>>
>>>> - Is the query code supposed to always return NULL in case of error?
>>>
>>> Yes.  If not, that is a bug in qmp_query_spice.
>>
>> Correct.
>>
>>>> In the
>>>>    case of hmp_info_spice, there is no check for info==NULL, so on the
>>
>> I'm blind.  Where?
>
> In hmp_info_spice, there is this code:
>
>     info = qmp_query_spice(NULL);
>
>     if (!info->enabled) {
>         monitor_printf(mon, "Server: disabled\n");
>         goto out;
>     }
>
> I guess this code relies on qmp_query_spice never returning an error.
> Why is that a safe assumption?

It's safe only because qmp_query_spice() never fails.

Of course, when you don't expect failure, you should *not* ignore it!
Pass &error_abort.  If you're right, you're no worse off, and if you're
wrong, you get your error pointed out clearly and reliably.

> This came to my attention because I wanted to return an error and a NULL
> value for modular spice if the module is not available.

Then you get to adjust the caller accordingly.

[...]