[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: |
Christophe de Dinechin |
Subject: |
Re: Missing qapi_free_Type in error case for qapi generated code? |
Date: |
Wed, 29 Jul 2020 10:48:24 +0200 |
User-agent: |
mu4e 1.5.2; emacs 26.3 |
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.
>
> 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?
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.
>
>>> contrary, it seems to indicate that a non-null result is always expected,
>>> and that function does call qapi_free_SpiceInfo
>>
>> Calling qapi_free_SpiceInfo(NULL) is a safe no-op. Or if you expect
>> the function to always succeed, you could pass &error_abort as the
>> errp parameter.
>>
>>>
>>> - If not, is there an existing shortcut to generate the correct deallocation
>>> code for return types that need it? You can't just use
>>> qapi_free_%(c_type)s because that would generate an extra * character,
>>> i.e. I get "SpiceInfo *" and not "SpiceInfo".
>>
>> Ah, you're debating about editing scripts/qapi/commands.py. If
>> anything, an edit to add 'assert(!retval)' if qmp_COMMAND failed might
>> be smarter than trying to add code to free retval.
>
> This is more complicated than it may seem.
>
> The "natural" error value for a pointer-valued function is NULL. I'm
> confident the handlers use it. assert(!retval) should work.
>
> For functions returning something else, people may have different ideas
> on what to return on error. To make assert(!retval) work, they need to
> return something "falsish". I'm not ready to bet my own money on all of
> them doing that.
That's what I was thinking too. Glad to confirm I was not reading that code
too wrong.
>
> Aside: only functions in pragma returns-whitelist can return
> non-pointer.
>
>>> - If not, is there any good way to know if the type is a pointer type?
>>> (A quick look in cripts/qapi/types.py does not show anything obvious)
>
> No clean way exists, simply because there has been no need. So far,
> we've always found a reasonable way to generate code that works whether
> types are pointers in C or not.
>
>> Look at scripts/qapi/schema.py; each QAPI metatype has implementations
>> of .c_name and .c_type that determine how to represent that QAPI
>> object in C. You probably want c_name instead of c_type when
>> constructing the name of a qapi_free_FOO function, but that goes back
>> to my question of whether such a call is even needed.
>
> Method c_name() returns a string you can interpolate into C identifiers.
>
> Method c_type() returns a string you can use as C type in generated
> code.
That part I understood. I was looking for something like method c_basetype()
or something like that for the non-pointer type of a pointer.
>
> The QAPI scripts could use more comments.
+1.
--
Cheers,
Christophe de Dinechin (IRC c3d)