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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

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

You're right.

Let's change cleanup only for the default case, like this:

       default:
           error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                      "BlockDirtyBitmapMergeSource");
 +         g_free(*obj);
 +         *obj = NULL;
       }
   out_obj:
       visit_end_alternate(v, (void **)obj);
       if (err && visit_is_input(v)) {
           qapi_free_BlockDirtyBitmapMergeSource(*obj);
           *obj = NULL;
       }
   out:
       error_propagate(errp, err);
   }

Thanks!