[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Missing qapi_free_Type in error case for qapi generated code?
From: |
Markus Armbruster |
Subject: |
Re: Missing qapi_free_Type in error case for qapi generated code? |
Date: |
Wed, 29 Jul 2020 14:44:41 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
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.
[...]