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 <ebl...@redhat.com>
---
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 4ac0c3a..66c0075 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 *@obj 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 *@obj 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, *@obj is assigned with an allocation of @size
* bytes. For other visitors, *@obj is the object to visit. Set *@errp
- * on failure.
- *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * on failure. Returns true if *@obj 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: *@obj 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 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 ac19d9d..9642c24 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -48,14 +48,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);
}
''',
@@ -135,23 +140,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);
}
''',
@@ -161,16 +167,13 @@ void visit_type_%(c_name)s(Visitor *m, %(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 *m, %(c_name)s **obj, const char *name,
Error **errp)
{
Error *err = NULL;
GenericList *i, **prev;
+ bool allocated = obj && !*obj;
visit_start_list(m, name, &err);
if (err) {
@@ -186,6 +189,10 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj,
const char *name, Error
visit_end_list(m, err ? NULL : &err);
out:
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
error_propagate(errp, err);
}
''',
@@ -214,8 +221,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;
}
@@ -244,11 +252,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
@@ -270,8 +282,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;
}
@@ -331,10 +344,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);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 03cad64..2da1b8b 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
