[Qemu-devel] [PATCH RFC v3 17/20] qapi: Change visit_type_XXX() 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_XXX() 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    | 40 ++++++++++++++++++++++++++++------------
 qapi/qapi-visit-core.c    |  8 ++++++--
 scripts/qapi-visit.py     | 37 +++++++++++++++++++++++++++----------
 tests/test-qmp-commands.c | 15 +++++++--------
 4 files changed, 68 insertions(+), 32 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index d08115c..cd1431a 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,6 +27,25 @@
  * 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_XXX(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, and enum 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_XXX(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 +65,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_XXX() 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 the struct started by a previous visit_start_struct().
@@ -60,14 +79,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 the implicit struct started earlier.
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6d8ea95..9cd17f8 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 08dfd59..e4f7c89 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -46,14 +46,19 @@ static void visit_type_%(c_type)s_fields(Visitor *m, 
%(c_type)s **obj, Error **e
 static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error 
**errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
+    allocated = visit_start_implicit_struct(m, (void **)obj, 
sizeof(%(c_type)s), &err);
     if (!err) {
         if (!obj || *obj) {
             visit_type_%(c_type)s_fields(m, obj, &err);
         }
         visit_end_implicit_struct(m, err ? NULL : &err);
     }
+    if (allocated && err) {
+        g_free(*obj);
+        *obj = NULL;
+    }
     error_propagate(errp, err);
 }
 ''',
@@ -131,23 +136,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 *m, %(c_name)s **obj, const char *name, 
Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), 
&err);
+    allocated = visit_start_struct(m, (void **)obj, "%(name)s", name, 
sizeof(%(c_name)s), &err);
     if (!err) {
         if (*obj) {
             visit_type_%(c_name)s_fields(m, obj, &err);
         }
         visit_end_struct(m, err ? NULL : &err);
     }
+    if (allocated && err) {
+        qapi_free_%(c_name)s(*obj);
+        *obj = NULL;
+    }
     error_propagate(errp, err);
 }
 ''',
@@ -203,8 +209,9 @@ def gen_visit_alternate(name, variants):
 void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, 
Error **errp)
 {
     Error *err = NULL;
+    bool allocated;

-    visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
+    allocated = visit_start_implicit_struct(m, (void**) obj, 
sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -233,11 +240,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, 
const char *name, Error
     }
 out_obj:
     visit_end_implicit_struct(m, 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

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

-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), 
&err);
+    allocated = visit_start_struct(m, (void **)obj, "%(name)s", name, 
sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -321,10 +333,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, 
const char *name, Error
     }
 out_obj:
     visit_end_struct(m, 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 5181823..89a3b47 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -218,15 +218,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);
-- 
2.4.3