Eric Blake <ebl...@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 <arm...@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 <ebl...@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!
