Missing qapi_free_Type in error case for qapi generated code?

Missing qapi_free_Type in error case for qapi generated code?

[Christophe de Dinechin]

The qapi generated code for qmp_marshal_query_spice seems to be missing a
resource deallocation for "retval". For example, for SpiceInfo:

#if defined(CONFIG_SPICE)
void qmp_marshal_query_spice(QDict *args, QObject **ret, Error **errp)
{
    Error *err = NULL;
    bool ok = false;
    Visitor *v;
    SpiceInfo *retval;

    v = qobject_input_visitor_new(QOBJECT(args));
    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
        goto out;
    }
    ok = visit_check_struct(v, errp);
    visit_end_struct(v, NULL);
    if (!ok) {
        goto out;
    }

    retval = qmp_query_spice(&err);
    error_propagate(errp, err);
    if (err) {
/* retval not freed here */
/* Missing: qapi_free_SpiceInfo(retval); */
        goto out;
    }

    qmp_marshal_output_SpiceInfo(retval, ret, errp);

out:
    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? In the
  case of hmp_info_spice, there is no check for info==NULL, so on the
  contrary, it seems to indicate that a non-null result is always expected,
  and that function does call qapi_free_SpiceInfo

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

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

--
Cheers,
Christophe de Dinechin (IRC c3d)

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

[Eric Blake]

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.

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

> In the
>    case of hmp_info_spice, there is no check for info==NULL, so on the
>    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.

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

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

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.

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

[Christophe de Dinechin]

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)

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

[Markus Armbruster]

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.

[...]