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

[Eric Blake]

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 <address@hidden>
> > ---
> >   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 <address@hidden>

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