Re: [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type

[Eric Blake <eblake@redhat.com>]

On 4/23/20 1:06 PM, Eric Blake wrote:
> On 4/23/20 11:00 AM, Markus Armbruster wrote:
>> An alternate type's visit_type_FOO() fails when it runs into an
>> invalid ->type.  If it's an input visit, we then need to free the the
>> object we got from visit_start_alternate().  We do that with
>> qapi_free_FOO(), which uses the dealloc visitor.
>>
>> Trouble is that object is in a bad state: its ->type is invalid.  So
>> the dealloc visitor will run into the same error again, and the error
>> recovery skips deallocating the alternate's (invalid) alternative.
>> This is a roundabout way to g_free() the alternate.
>>
>> Simplify: replace the qapi_free_FOO() by g_free().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   scripts/qapi/visit.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
> 
> Required looking at what gets generated into qapi_free_FOO() as well as 
> when visit_start_alternate() can fail, but makes sense.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Actually, I'm having second thoughts.  As an example, look at the generated:

> void visit_type_BlockDirtyBitmapMergeSource(Visitor *v, const char *name, BlockDirtyBitmapMergeSource **obj, Error **errp)
> {
>     Error *err = NULL;
> 
>     visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
>                           &err);
>     if (err) {
>         goto out;
>     }
>     if (!*obj) {
>         goto out_obj;
[1]
>     }
>     switch ((*obj)->type) {
>     case QTYPE_QSTRING:
>         visit_type_str(v, name, &(*obj)->u.local, &err);
[2]
>         break;
>     case QTYPE_QDICT:
>         visit_start_struct(v, name, NULL, 0, &err);
>         if (err) {
>             break;
[3]
>         }
>         visit_type_BlockDirtyBitmap_members(v, &(*obj)->u.external, &err);
>         if (!err) {
>             visit_check_struct(v, &err);
[4]
>         }
>         visit_end_struct(v, NULL);
>         break;
>     case QTYPE_NONE:
>         abort();
>     default:
>         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                    "BlockDirtyBitmapMergeSource");
[5]
>     }
> out_obj:
>     visit_end_alternate(v, (void **)obj);
>     if (err && visit_is_input(v)) {
>         qapi_free_BlockDirtyBitmapMergeSource(*obj);

If we got here, we must have failed at any of the points mentioned above.

If [1], visit_start_alternate() failed, but *obj is NULL and both 
qapi_free_FOO(NULL) and g_free(NULL) are safe.

If [2], visit_type_str() failed, so *obj is allocated but the embedded 
string (here, u.local) was left NULL.  qapi_free_FOO() then does nothing 
further than g_free(obj).

If [3], visit_start_struct() failed, the embedded dict (here, 
u.external) was left NULL.  qapi_free_FOO() then does nothing further 
than g_free(obj).

If [5], we have the wrong ->type.  As pointed out by this commit, 
qapi_free_FOO() does nothing further than g_free(obj).

But what happens in [4]?  Here, the embedded dict was allocated, but we 
then failed while parsing its members.  That leaves us in a 
partially-allocated state, and g_free(NULL) does NOT recursively visit 
that partial allocation.  I think this patch is prone to a memory leak 
unless you _also_ patch things to free any dict branch on failure 
(perhaps during the QTYPE_QDICT case label, rather than here at the end).

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