[v2] qapi: Spring cleaning

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

Posted by Markus Armbruster 5 years, 9 months ago

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.
Works, because qapi_free_FOO() ignores the error.

Avoid it instead: free the messed up object with by g_free().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/visit.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index e3467b770b..678109dfb5 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -234,6 +234,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     default:
         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "%(name)s");
+        /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
+        g_free(*obj);
+        *obj = NULL;
     }
 out_obj:
     visit_end_alternate(v, (void **)obj);
-- 
2.21.1

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

Posted by Eric Blake 5 years, 9 months ago

On 4/24/20 3:43 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.
> Works, because qapi_free_FOO() ignores the error.
> 
> Avoid it instead: free the messed up object with by g_free().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/visit.py | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index e3467b770b..678109dfb5 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -234,6 +234,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>       default:
>           error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                      "%(name)s");
> +        /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
> +        g_free(*obj);
> +        *obj = NULL;

Yep, fixes the problem I spotted in v1, and much easier to review with 
the reduced scope of change.

Reviewed-by: Eric Blake <eblake@redhat.com>

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