[Qemu-devel] [PATCH v5 29/46] qapi: Change visit_type_FOO() to no longer return partial objects

[Eric Blake]

Returning a partial object on error is an invitation for a careless
caller to leak memory.  As no one outside the testsuite was actually
relying on these semantics, it is cleaner to just document and
guarantee that ALL visit_type_FOO() functions do not alter *obj
when an error is encountered during an input visitor.

Signed-off-by: Eric Blake <address@hidden>
---
 include/qapi/visitor.h         | 45 +++++++++++++++++++++++++++++------------
 qapi/qapi-visit-core.c         |  8 ++++++--
 scripts/qapi-visit.py          | 46 +++++++++++++++++++++++++++++-------------
 tests/test-qmp-commands.c      | 15 +++++++-------
 tests/test-qmp-input-visitor.c | 29 +++++++++++---------------
 5 files changed, 89 insertions(+), 54 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 284a978..69935fa 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,6 +27,26 @@
  * scripts/qapi-visit.py.  For the visitor callback contracts, see
  * visitor-impl.h. */

+/* All qapi types have a corresponding function with a signature
+ * compatible with this:
+ *
+ * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp);
+ *
+ * where address@hidden is itself a pointer or a scalar.  (The visit functions 
for
+ * built-in types are declared here, while the functions for qapi-defined
+ * struct, union, enum, and list types are generated; see qapi-visit.h).
+ * Input visitors populate address@hidden on success, and leave it unchanged on
+ * failure.
+ *
+ * Additionally, all qapi structs have a generated function compatible
+ * with this:
+ *
+ * void qapi_free_FOO(void *obj);
+ *
+ * which behaves like free(), even if @obj is NULL or was only partially
+ * allocated before encountering an error.
+ */
+
 /* This struct is layout-compatible with all other *List structs
  * created by the qapi generator. */
 typedef struct GenericList
@@ -46,12 +66,12 @@ typedef struct GenericList
  * input visitor, @obj can be NULL to validate that the visit will
  * succeed; otherwise, address@hidden is assigned with an allocation of @size
  * bytes. For other visitors, address@hidden is the object to visit. Set 
address@hidden
- * on failure.
- *
- * FIXME: address@hidden can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * on failure.  Returns true if address@hidden was allocated; if that happens,
+ * and an error occurs any time before the matching visit_end_struct(),
+ * then the caller (usually a visit_type_FOO() function) knows to undo
+ * the allocation before returning control further.
  */
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp);
 /**
  * Complete a struct started earlier.
@@ -62,14 +82,11 @@ void visit_end_struct(Visitor *v, Error **errp);

 /**
  * Prepare to visit an implicit struct.
- * Similar to visit_start_struct(), except that this will visit a
- * C pointer pointing to @size bytes, and where the QDict fields are
- * part of the parent object.
- *
- * FIXME: address@hidden can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * Similar to visit_start_struct(), including return semantics, except
+ * that this will visit a C pointer pointing to @size bytes, and where
+ * the QDict fields are part of the parent object.
  */
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp);
 /**
  * Complete an implicit struct started earlier.
@@ -93,7 +110,9 @@ void visit_start_list(Visitor *v, const char *name, Error 
**errp);
  * loop until a NULL return or error occurs; for each non-NULL return,
  * the caller must then call the appropriate visit_type_*() for the
  * element type of the list, with that function's name parameter set
- * to NULL.
+ * to NULL.  If an error occurs, then the caller (usually a
+ * visit_type_FOO() function) knows to undo the list allocat04ion before
+ * returning control further.
  */
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
 /**
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 020ea34..d885347 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -17,10 +17,12 @@
 #include "qapi/visitor.h"
 #include "qapi/visitor-impl.h"

-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp)
 {
+    bool track_allocation = obj && !*obj;
     v->start_struct(v, obj, kind, name, size, errp);
+    return track_allocation && *obj;
 }

 void visit_end_struct(Visitor *v, Error **errp)
@@ -28,12 +30,14 @@ void visit_end_struct(Visitor *v, Error **errp)
     v->end_struct(v, errp);
 }

-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp)
 {
+    bool track_allocation = obj && !*obj;
     if (v->start_implicit_struct) {
         v->start_implicit_struct(v, obj, size, errp);
     }
+    return track_allocation && *obj;
 }

 void visit_end_implicit_struct(Visitor *v, Error **errp)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d94b34e..898a32b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -48,14 +48,19 @@ static void visit_type_%(c_type)s_fields(Visitor *v, 
%(c_type)s **obj, Error **e
 static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error 
**errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
+    allocated = visit_start_implicit_struct(v, (void **)obj, 
sizeof(%(c_type)s), &err);
     if (!err) {
         if (!obj || *obj) {
             visit_type_%(c_type)s_fields(v, obj, &err);
         }
         visit_end_implicit_struct(v, err ? NULL : &err);
     }
+    if (allocated && err) {
+        g_free(*obj);
+        *obj = NULL;
+    }
     error_propagate(errp, err);
 }
 ''',
@@ -108,23 +113,24 @@ out:
 def gen_visit_struct(name, base, members):
     ret = gen_visit_struct_fields(name, base, members)

-    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
-    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
-    # rather than leaving it non-NULL. As currently written, the caller must
-    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret += mcgen('''

 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), 
&err);
+    allocated = visit_start_struct(v, (void **)obj, "%(name)s", name, 
sizeof(%(c_name)s), &err);
     if (!err) {
         if (*obj) {
             visit_type_%(c_name)s_fields(v, obj, &err);
         }
         visit_end_struct(v, err ? NULL : &err);
     }
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
     error_propagate(errp, err);
 }
 ''',
@@ -134,16 +140,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
const char *name, Error


 def gen_visit_list(name, element_type):
-    # FIXME: if *obj is NULL on entry, and the first visit_next_list()
-    # assigns to *obj, while a later one fails, we should clean up *obj
-    # rather than leaving it non-NULL. As currently written, the caller must
-    # call qapi_free_FOOList() to avoid a memory leak of the partial FOOList.
     return mcgen('''

 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
Error **errp)
 {
     Error *err = NULL;
     GenericList *i, **prev;
+    bool allocated = obj && !*obj;

     visit_start_list(v, name, &err);
     if (err) {
@@ -159,6 +162,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
const char *name, Error

     visit_end_list(v, err ? NULL : &err);
 out:
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
     error_propagate(errp, err);
 }
 ''',
@@ -187,8 +194,9 @@ def gen_visit_alternate(name, variants):
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
+    allocated = visit_start_implicit_struct(v, (void **)obj, 
sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -217,11 +225,15 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
const char *name, Error
     }
 out_obj:
     visit_end_implicit_struct(v, err ? NULL : &err);
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
 out:
     error_propagate(errp, err);
 }
 ''',
-                 name=name)
+                 name=name, c_name=c_name(name))

     return ret

@@ -243,8 +255,9 @@ def gen_visit_union(name, base, variants):
 void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), 
&err);
+    allocated = visit_start_struct(v, (void **)obj, "%(name)s", name, 
sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -311,10 +324,15 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, 
const char *name, Error
     }
 out_obj:
     visit_end_struct(v, err ? NULL : &err);
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
 out:
     error_propagate(errp, err);
 }
-''')
+''',
+                 c_name=c_name(name))

     return ret

diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 8d5249e..681ec65 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -223,15 +223,14 @@ static void test_dealloc_partial(void)
         QDECREF(ud2_dict);
     }

-    /* verify partial success */
-    assert(ud2 != NULL);
-    assert(ud2->string0 != NULL);
-    assert(strcmp(ud2->string0, text) == 0);
-    assert(ud2->dict1 == NULL);
-
-    /* confirm & release construction error */
-    assert(err != NULL);
+    /* verify that visit_type_XXX() cleans up properly on error */
+    assert(err);
     error_free(err);
+    assert(!ud2);
+
+    /* Manually create a partial object, leaving ud2->dict1 at NULL */
+    ud2 = g_new0(UserDefTwo, 1);
+    ud2->string0 = g_strdup(text);

     /* tear down partial object */
     qapi_free_UserDefTwo(ud2);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 667301c..bc9ed04 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -201,9 +201,10 @@ static void visit_type_TestStruct(Visitor *v, TestStruct 
**obj,
                                   const char *name, Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
-                       &err);
+    allocated = visit_start_struct(v, (void **)obj, "TestStruct", name,
+                                   sizeof(TestStruct), &err);
     if (err) {
         goto out;
     }
@@ -218,9 +219,12 @@ static void visit_type_TestStruct(Visitor *v, TestStruct 
**obj,
     visit_type_str(v, &(*obj)->string, "string", &err);

 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v, err ? NULL : &err);
+    if (allocated && err) {
+        g_free((*obj)->string);
+        g_free(*obj);
+        *obj = NULL;
+    }
 out:
     error_propagate(errp, err);
 }
@@ -825,24 +829,15 @@ static void test_visitor_in_errors(TestInputVisitorData 
*data,

     visit_type_TestStruct(v, &p, NULL, &err);
     g_assert(err);
-    /* FIXME - a failed parse should not leave a partially-allocated p
-     * for us to clean up; this could cause callers to leak memory. */
-    g_assert(p->string == NULL);
-
+    g_assert(!p);
     error_free(err);
     err = NULL;
-    g_free(p->string);
-    g_free(p);
     visitor_input_teardown(data, NULL);

     v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]");
-    /* FIXME - a failed parse should not leave a partially-allocated
-     * array for us to clean up; this could cause callers to leak
-     * memory. */
     visit_type_strList(v, &q, NULL, &err);
-    assert(q);
-    assert(err);
-    qapi_free_strList(q);
+    g_assert(!q);
+    g_assert(err);
     error_free(err);
     visitor_input_teardown(data, NULL);
 }
-- 
2.4.3