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

[Markus Armbruster <armbru@redhat.com>]

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.

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?

>>    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.

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.

The QAPI scripts could use more comments.