| Message ID | 1450717720-9627-35-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> wrote: > 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 pointer-based visit_type_FOO() functions always > leave a safe value in *obj during an input visitor (either the new > object on success, or NULL if an error is encountered). > > Since input visitors have blind assignment semantics, we have to > track the result of whether an assignment is made all the way down > to each visitor callback implementation, to avoid making decisions > based on potentially uninitialized storage. > > Note that we still leave *obj unchanged after a scalar-based > visit_type_FOO(); I did not feel like auditing all uses of > visit_type_Enum() to see if the callers would tolerate a specific > sentinel value (not to mention having to decide whether it would > be better to use 0 or ENUM__MAX as that sentinel). > > Signed-off-by: Eric Blake <eblake@redhat.com> > nice cleanup, few issues: > --- > v8: rebase to earlier changes > v7: rebase to earlier changes, enhance commit message, also fix > visit_type_str() and visit_type_any() > v6: rebase on top of earlier doc and formatting improvements, mention > that *obj can be uninitialized on entry to an input visitor, rework > semantics to keep valgrind happy on uninitialized input, break some > long lines > --- > include/qapi/visitor-impl.h | 6 ++--- > include/qapi/visitor.h | 53 ++++++++++++++++++++++++++++-------------- > qapi/opts-visitor.c | 11 ++++++--- > qapi/qapi-dealloc-visitor.c | 9 ++++--- > qapi/qapi-visit-core.c | 39 ++++++++++++++++++++++++++----- > qapi/qmp-input-visitor.c | 18 +++++++++----- > qapi/qmp-output-visitor.c | 6 +++-- > qapi/string-input-visitor.c | 6 +++-- > qapi/string-output-visitor.c | 3 ++- > scripts/qapi-visit.py | 40 +++++++++++++++++++++++-------- > tests/test-qmp-commands.c | 13 +++++------ > tests/test-qmp-input-strict.c | 19 +++++++-------- > tests/test-qmp-input-visitor.c | 10 ++------ > 13 files changed, 154 insertions(+), 79 deletions(-) > > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 94d65fa..c32e5f5 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -26,7 +26,7 @@ struct Visitor > { > /* Must be provided to visit structs (the string visitors do not > * currently visit structs). */ > - void (*start_struct)(Visitor *v, const char *name, void **obj, > + bool (*start_struct)(Visitor *v, const char *name, void **obj, > size_t size, Error **errp); > /* May be NULL; most useful for input visitors. */ > void (*check_struct)(Visitor *v, Error **errp); > @@ -34,13 +34,13 @@ struct Visitor > void (*end_struct)(Visitor *v); > > /* May be NULL; most useful for input visitors. */ > - void (*start_implicit_struct)(Visitor *v, void **obj, size_t size, > + bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size, > Error **errp); > /* May be NULL */ > void (*end_implicit_struct)(Visitor *v); > > /* Must be set */ > - void (*start_list)(Visitor *v, const char *name, GenericList **list, > + bool (*start_list)(Visitor *v, const char *name, GenericList **list, > Error **errp); > /* Must be set */ > GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp); > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index 4638863..4eae633 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -31,6 +31,27 @@ > * visitor-impl.h. > */ > > +/* All qapi types have a corresponding function with a signature > + * roughly 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 may receive an uninitialized *@obj, and guarantee a > + * safe value is assigned (a new object on success, or NULL on failure). > + * Output visitors expect *@obj to be properly initialized on entry. > + * > + * 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 > @@ -62,11 +83,12 @@ typedef struct GenericList > * Set *@errp on failure; for example, if the input stream does not > * have a member @name or if the member is not an object. > * > - * FIXME: For input visitors, *@obj can be assigned here even if later > - * visits will fail; this can lead to memory leaks if clients aren't > - * careful. > + * 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, const char *name, void **obj, > +bool visit_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp); > /** > * Prepare for completing a struct visit. > @@ -85,19 +107,15 @@ void visit_end_struct(Visitor *v); > > /** > * Prepare to visit an implicit struct. > - * Similar to visit_start_struct(), except that an implicit struct > - * represents a subset of keys that are present at the same nesting level > - * of a common object but as a separate qapi C struct, rather than a new > - * object at a deeper nesting level. > + * Similar to visit_start_struct(), including return semantics, except > + * that an implicit struct represents a subset of keys that are present > + * at the same nesting level of a common object but as a separate qapi > + * C struct, rather than a new object at a deeper nesting level. > * > * @obj must not be NULL, since this function is only called when > * visiting with qapi structs. > - * > - * FIXME: For input visitors, *@obj can be assigned here even if later > - * visits will fail; this can lead to memory leaks if clients aren't > - * careful. > */ > -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 visit started earlier. > @@ -126,11 +144,12 @@ void visit_end_implicit_struct(Visitor *v); > * in this case, visit_next_list() will not be needed, but > * visit_end_list() is still mandatory. > * > - * FIXME: For input visitors, *@list can be assigned here even if > - * later visits will fail; this can lead to memory leaks if clients > - * aren't careful. > + * Returns true if *@list was allocated; if that happens, and an error > + * occurs any time before the matching visit_end_list(), then the > + * caller (usually a visit_type_FOO() function) knows to undo the > + * allocation before returning control further. > */ > -void visit_start_list(Visitor *v, const char *name, GenericList **list, > +bool visit_start_list(Visitor *v, const char *name, GenericList **list, > Error **errp); > /** > * Iterate over a GenericList during a list visit. > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index c5a7396..38d1e68 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -122,18 +122,20 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) > } > > > -static void > +static bool > opts_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > OptsVisitor *ov = to_ov(v); > const QemuOpt *opt; > + bool result = false; > > if (obj) { > *obj = g_malloc0(size > 0 ? size : 1); > + result = true; > } > if (ov->depth++ > 0) { > - return; > + return result; > } > > ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, > @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, > ov->fake_id_opt->str = g_strdup(ov->opts_root->id); > opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); > } > + return result; > } > > > @@ -210,7 +213,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) > } > > > -static void > +static bool > opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > { > OptsVisitor *ov = to_ov(v); > @@ -223,8 +226,10 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > if (ov->repeated_opts) { > ov->list_mode = LM_IN_PROGRESS; > *list = g_new0(GenericList, 1); > + return true; > } else { > *list = NULL; > + return false; > } > } > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 839049e..0990199 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -53,11 +53,12 @@ static void *qapi_dealloc_pop(QapiDeallocVisitor *qov) > return value; > } > > -static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, > +static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, > size_t unused, Error **errp) > { > QapiDeallocVisitor *qov = to_qov(v); > qapi_dealloc_push(qov, obj); > + return false; > } > > static void qapi_dealloc_end_struct(Visitor *v) > @@ -69,13 +70,14 @@ static void qapi_dealloc_end_struct(Visitor *v) > } > } > > -static void qapi_dealloc_start_implicit_struct(Visitor *v, > +static bool qapi_dealloc_start_implicit_struct(Visitor *v, > void **obj, > size_t size, > Error **errp) > { > QapiDeallocVisitor *qov = to_qov(v); > qapi_dealloc_push(qov, obj); > + return false; > } > > static void qapi_dealloc_end_implicit_struct(Visitor *v) > @@ -87,11 +89,12 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v) > } > } > > -static void qapi_dealloc_start_list(Visitor *v, const char *name, > +static bool qapi_dealloc_start_list(Visitor *v, const char *name, > GenericList **list, Error **errp) > { > QapiDeallocVisitor *qov = to_qov(v); > qapi_dealloc_push(qov, NULL); > + return false; > } > > static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list, > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index f391a70..977df85 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -26,14 +26,20 @@ static bool visit_is_output(Visitor *v) > return v->type_enum == output_type_enum; > } > > -void visit_start_struct(Visitor *v, const char *name, void **obj, > +bool visit_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > + bool result; > + > assert(obj ? size : !size); > if (obj && visit_is_output(v)) { > assert(*obj); > } > - v->start_struct(v, name, obj, size, errp); > + result = v->start_struct(v, name, obj, size, errp); > + if (result) { > + assert(obj && *obj); > + } > + return result; > } > > void visit_check_struct(Visitor *v, Error **errp) > @@ -48,16 +54,22 @@ void visit_end_struct(Visitor *v) > v->end_struct(v); > } > > -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 result = false; > + > assert(obj && size); > if (visit_is_output(v)) { > assert(*obj); > } > if (v->start_implicit_struct) { > - v->start_implicit_struct(v, obj, size, errp); > + result = v->start_implicit_struct(v, obj, size, errp); > } > + if (result) { > + assert(*obj); > + } > + return result; > } > > void visit_end_implicit_struct(Visitor *v) > @@ -67,10 +79,14 @@ void visit_end_implicit_struct(Visitor *v) > } > } > > -void visit_start_list(Visitor *v, const char *name, GenericList **list, > +bool visit_start_list(Visitor *v, const char *name, GenericList **list, > Error **errp) > { > - v->start_list(v, name, list, errp); > + bool result = v->start_list(v, name, list, errp); > + if (result) { > + assert(list && *list); > + } > + return result; > } > > GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp) > @@ -229,6 +245,7 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) > > void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) > { > + Error *err = NULL; > assert(obj); > /* TODO: Fix callers to not pass NULL when they mean "", so that we > * can enable: > @@ -237,6 +254,10 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) > } > */ > v->type_str(v, name, obj, errp); > + if (!visit_is_output(v) && err) { > + *obj = NULL; > + } This will always evelatute to false, you need to change the line above I suppose > + error_propagate(errp, err); > } > > void visit_type_number(Visitor *v, const char *name, double *obj, > @@ -248,11 +269,17 @@ void visit_type_number(Visitor *v, const char *name, double *obj, > > void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) > { > + Error *err = NULL; > + > assert(obj); > if (visit_is_output(v)) { > assert(*obj); > } > v->type_any(v, name, obj, errp); > + if (!visit_is_output(v) && err) { > + *obj = NULL; > + } same here > + error_propagate(errp, err); > } > > void visit_type_null(Visitor *v, const char *name, Error **errp) > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 550ee93..2427a80 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -130,7 +130,7 @@ static void qmp_input_pop(Visitor *v) > qiv->nb_stack--; > } > > -static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > +static bool qmp_input_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > @@ -140,30 +140,34 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "QDict"); > - return; > + return false; > } > > qmp_input_push(qiv, qobj, NULL, &err); > if (err) { > error_propagate(errp, err); > - return; > + return false; > } > > if (obj) { > *obj = g_malloc0(size); > + return true; > } > + return false; > } > > > -static void qmp_input_start_implicit_struct(Visitor *v, void **obj, > +static bool qmp_input_start_implicit_struct(Visitor *v, void **obj, > size_t size, Error **errp) > { > if (obj) { > *obj = g_malloc0(size); > + return true; > } > + return false; > } > > -static void qmp_input_start_list(Visitor *v, const char *name, > +static bool qmp_input_start_list(Visitor *v, const char *name, > GenericList **list, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > @@ -173,7 +177,7 @@ static void qmp_input_start_list(Visitor *v, const char *name, > if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "list"); > - return; > + return false; > } > > entry = qlist_first(qobject_to_qlist(qobj)); > @@ -181,10 +185,12 @@ static void qmp_input_start_list(Visitor *v, const char *name, > if (list) { > if (entry) { > *list = g_new0(GenericList, 1); > + return true; > } else { > *list = NULL; > } > } > + return false; > } > > static GenericList *qmp_input_next_list(Visitor *v, GenericList *list, > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index c9615fe..cd45ffb 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -103,7 +103,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, > } > } > > -static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, > +static bool qmp_output_start_struct(Visitor *v, const char *name, void **obj, > size_t unused, Error **errp) > { > QmpOutputVisitor *qov = to_qov(v); > @@ -111,6 +111,7 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, > > qmp_output_add(qov, name, dict); > qmp_output_push(qov, dict); > + return false; > } > > static void qmp_output_end_struct(Visitor *v) > @@ -119,7 +120,7 @@ static void qmp_output_end_struct(Visitor *v) > qmp_output_pop(qov); > } > > -static void qmp_output_start_list(Visitor *v, const char *name, > +static bool qmp_output_start_list(Visitor *v, const char *name, > GenericList **listp, Error **errp) > { > QmpOutputVisitor *qov = to_qov(v); > @@ -127,6 +128,7 @@ static void qmp_output_start_list(Visitor *v, const char *name, > > qmp_output_add(qov, name, list); > qmp_output_push(qov, list); > + return false; > } > > static GenericList *qmp_output_next_list(Visitor *v, GenericList *list, > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 582a62a..0e4a07c 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -120,7 +120,7 @@ error: > siv->ranges = NULL; > } > > -static void > +static bool > start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > { > StringInputVisitor *siv = to_siv(v); > @@ -132,7 +132,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > parse_str(siv, &err); > if (err) { > error_propagate(errp, err); > - return; > + return false; > } > > siv->cur_range = g_list_first(siv->ranges); > @@ -142,8 +142,10 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > siv->cur = r->begin; > } > *list = g_new0(GenericList, 1); > + return true; > } else { > *list = NULL; > + return false; > } > } > > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c > index 7209d80..2666802 100644 > --- a/qapi/string-output-visitor.c > +++ b/qapi/string-output-visitor.c > @@ -263,7 +263,7 @@ static void print_type_number(Visitor *v, const char *name, double *obj, > string_output_set(sov, g_strdup_printf("%f", *obj)); > } > > -static void > +static bool > start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > { > StringOutputVisitor *sov = to_sov(v); > @@ -276,6 +276,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > if (*list && (*list)->next) { > sov->list_mode = LM_STARTED; > } > + return false; > } > > static GenericList * > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 6016734..eebb5f7 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -58,8 +58,10 @@ def gen_visit_implicit_struct(typ): > 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) { > goto out; > } > @@ -69,6 +71,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * > visit_type_%(c_type)s_fields(v, obj, &err); > out_obj: > visit_end_implicit_struct(v); > + if (allocated && err) { > + g_free(*obj); > + *obj = NULL; > + } > out: > error_propagate(errp, err); > } > @@ -116,18 +122,15 @@ out: > > > 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, const char *name, %(c_name)s **obj, Error **errp) > { > Error *err = NULL; > %(c_name)s *elt; > + bool allocated; > > - visit_start_list(v, name, (GenericList **)obj, &err); > + allocated = visit_start_list(v, name, (GenericList **)obj, &err); > if (err) { > goto out; > } > @@ -144,6 +147,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > } > visit_end_list(v); > out: > + if (allocated && err) { > + qapi_free_%(c_name)s(*obj); > + *obj = NULL; > + } > error_propagate(errp, err); > } > ''', > @@ -174,8 +181,10 @@ def gen_visit_alternate(name, variants): > void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 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; > } > @@ -204,11 +213,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > } > out_obj: > visit_end_implicit_struct(v); > + 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 > > @@ -236,8 +249,10 @@ def gen_visit_object(name, base, members, variants): > void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) > { > Error *err = NULL; > + bool allocated; > > - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); > + allocated = visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), > + &err); > if (err) { > goto out; > } > @@ -301,10 +316,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > visit_check_struct(v, &err); > out_obj: > visit_end_struct(v); > + 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 bc8085d..d3466a4 100644 > --- a/tests/test-qmp-commands.c > +++ b/tests/test-qmp-commands.c > @@ -227,14 +227,13 @@ 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 */ > + /* verify that visit_type_XXX() cleans up properly on error */ > error_free_or_abort(&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-strict.c b/tests/test-qmp-input-strict.c > index 775ad39..4db35dd 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -181,10 +181,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data, > > visit_type_TestStruct(v, NULL, &p, &err); > error_free_or_abort(&err); > - if (p) { > - g_free(p->string); > - } > - g_free(p); > + g_assert(!p); > } > > static void test_validate_fail_struct_nested(TestInputVisitorData *data, > @@ -198,7 +195,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data, > > visit_type_UserDefTwo(v, NULL, &udp, &err); > error_free_or_abort(&err); > - qapi_free_UserDefTwo(udp); > + g_assert(!udp); > } > > static void test_validate_fail_list(TestInputVisitorData *data, > @@ -212,7 +209,7 @@ static void test_validate_fail_list(TestInputVisitorData *data, > > visit_type_UserDefOneList(v, NULL, &head, &err); > error_free_or_abort(&err); > - qapi_free_UserDefOneList(head); > + g_assert(!head); > } > > static void test_validate_fail_union_native_list(TestInputVisitorData *data, > @@ -227,7 +224,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data, > > visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err); > error_free_or_abort(&err); > - qapi_free_UserDefNativeListUnion(tmp); > + g_assert(!tmp); > } > > static void test_validate_fail_union_flat(TestInputVisitorData *data, > @@ -241,7 +238,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data, > > visit_type_UserDefFlatUnion(v, NULL, &tmp, &err); > error_free_or_abort(&err); > - qapi_free_UserDefFlatUnion(tmp); > + g_assert(!tmp); > } > > static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, > @@ -256,13 +253,13 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, > > visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err); > error_free_or_abort(&err); > - qapi_free_UserDefFlatUnion2(tmp); > + g_assert(!tmp); > } > > static void test_validate_fail_alternate(TestInputVisitorData *data, > const void *unused) > { > - UserDefAlternate *tmp = NULL; > + UserDefAlternate *tmp; > Visitor *v; > Error *err = NULL; > > @@ -270,7 +267,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data, > > visit_type_UserDefAlternate(v, NULL, &tmp, &err); > error_free_or_abort(&err); > - qapi_free_UserDefAlternate(tmp); > + g_assert(!tmp); > } > > static void do_test_validate_qmp_introspect(TestInputVisitorData *data, > diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c > index f6bd408..7f9191c 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -708,18 +708,12 @@ static void test_visitor_in_errors(TestInputVisitorData *data, > > visit_type_TestStruct(v, NULL, &p, &err); > error_free_or_abort(&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_free(p->string); > - g_free(p); > + g_assert(!p); > > v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]"); > visit_type_strList(v, NULL, &q, &err); > error_free_or_abort(&err); > - assert(q); > - qapi_free_strList(q); > + assert(!q); > } > > static void test_visitor_in_wrong_type(TestInputVisitorData *data, > -- > 2.4.3 > >
On 01/05/2016 10:22 AM, Marc-André Lureau wrote: > Hi > > On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> wrote: >> 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 pointer-based visit_type_FOO() functions always >> leave a safe value in *obj during an input visitor (either the new >> object on success, or NULL if an error is encountered). >> >> Since input visitors have blind assignment semantics, we have to >> track the result of whether an assignment is made all the way down >> to each visitor callback implementation, to avoid making decisions >> based on potentially uninitialized storage. >> >> Note that we still leave *obj unchanged after a scalar-based >> visit_type_FOO(); I did not feel like auditing all uses of >> visit_type_Enum() to see if the callers would tolerate a specific >> sentinel value (not to mention having to decide whether it would >> be better to use 0 or ENUM__MAX as that sentinel). >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> > > nice cleanup, few issues: > >> @@ -237,6 +254,10 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) >> } >> */ >> v->type_str(v, name, obj, errp); >> + if (!visit_is_output(v) && err) { >> + *obj = NULL; >> + } > > This will always evelatute to false, you need to change the line above I suppose > >> + error_propagate(errp, err); Oh right, that needs to be v->type_str(..., &err). I'll have to double-check that no assertions trigger with the fixed code, and provide the fixup patch. I don't know if Markus will want me to spin a v9, but I'll wait for his comments before deciding if a full respin is needed.
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 94d65fa..c32e5f5 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -26,7 +26,7 @@ struct Visitor { /* Must be provided to visit structs (the string visitors do not * currently visit structs). */ - void (*start_struct)(Visitor *v, const char *name, void **obj, + bool (*start_struct)(Visitor *v, const char *name, void **obj, size_t size, Error **errp); /* May be NULL; most useful for input visitors. */ void (*check_struct)(Visitor *v, Error **errp); @@ -34,13 +34,13 @@ struct Visitor void (*end_struct)(Visitor *v); /* May be NULL; most useful for input visitors. */ - void (*start_implicit_struct)(Visitor *v, void **obj, size_t size, + bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size, Error **errp); /* May be NULL */ void (*end_implicit_struct)(Visitor *v); /* Must be set */ - void (*start_list)(Visitor *v, const char *name, GenericList **list, + bool (*start_list)(Visitor *v, const char *name, GenericList **list, Error **errp); /* Must be set */ GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp); diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 4638863..4eae633 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -31,6 +31,27 @@ * visitor-impl.h. */ +/* All qapi types have a corresponding function with a signature + * roughly 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 may receive an uninitialized *@obj, and guarantee a + * safe value is assigned (a new object on success, or NULL on failure). + * Output visitors expect *@obj to be properly initialized on entry. + * + * 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 @@ -62,11 +83,12 @@ typedef struct GenericList * Set *@errp on failure; for example, if the input stream does not * have a member @name or if the member is not an object. * - * FIXME: For input visitors, *@obj can be assigned here even if later - * visits will fail; this can lead to memory leaks if clients aren't - * careful. + * 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, const char *name, void **obj, +bool visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp); /** * Prepare for completing a struct visit. @@ -85,19 +107,15 @@ void visit_end_struct(Visitor *v); /** * Prepare to visit an implicit struct. - * Similar to visit_start_struct(), except that an implicit struct - * represents a subset of keys that are present at the same nesting level - * of a common object but as a separate qapi C struct, rather than a new - * object at a deeper nesting level. + * Similar to visit_start_struct(), including return semantics, except + * that an implicit struct represents a subset of keys that are present + * at the same nesting level of a common object but as a separate qapi + * C struct, rather than a new object at a deeper nesting level. * * @obj must not be NULL, since this function is only called when * visiting with qapi structs. - * - * FIXME: For input visitors, *@obj can be assigned here even if later - * visits will fail; this can lead to memory leaks if clients aren't - * careful. */ -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 visit started earlier. @@ -126,11 +144,12 @@ void visit_end_implicit_struct(Visitor *v); * in this case, visit_next_list() will not be needed, but * visit_end_list() is still mandatory. * - * FIXME: For input visitors, *@list can be assigned here even if - * later visits will fail; this can lead to memory leaks if clients - * aren't careful. + * Returns true if *@list was allocated; if that happens, and an error + * occurs any time before the matching visit_end_list(), then the + * caller (usually a visit_type_FOO() function) knows to undo the + * allocation before returning control further. */ -void visit_start_list(Visitor *v, const char *name, GenericList **list, +bool visit_start_list(Visitor *v, const char *name, GenericList **list, Error **errp); /** * Iterate over a GenericList during a list visit. diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index c5a7396..38d1e68 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -122,18 +122,20 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) } -static void +static bool opts_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { OptsVisitor *ov = to_ov(v); const QemuOpt *opt; + bool result = false; if (obj) { *obj = g_malloc0(size > 0 ? size : 1); + result = true; } if (ov->depth++ > 0) { - return; + return result; } ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, ov->fake_id_opt->str = g_strdup(ov->opts_root->id); opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); } + return result; } @@ -210,7 +213,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) } -static void +static bool opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { OptsVisitor *ov = to_ov(v); @@ -223,8 +226,10 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) if (ov->repeated_opts) { ov->list_mode = LM_IN_PROGRESS; *list = g_new0(GenericList, 1); + return true; } else { *list = NULL; + return false; } } diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 839049e..0990199 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -53,11 +53,12 @@ static void *qapi_dealloc_pop(QapiDeallocVisitor *qov) return value; } -static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, +static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, size_t unused, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, obj); + return false; } static void qapi_dealloc_end_struct(Visitor *v) @@ -69,13 +70,14 @@ static void qapi_dealloc_end_struct(Visitor *v) } } -static void qapi_dealloc_start_implicit_struct(Visitor *v, +static bool qapi_dealloc_start_implicit_struct(Visitor *v, void **obj, size_t size, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, obj); + return false; } static void qapi_dealloc_end_implicit_struct(Visitor *v) @@ -87,11 +89,12 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v) } } -static void qapi_dealloc_start_list(Visitor *v, const char *name, +static bool qapi_dealloc_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, NULL); + return false; } static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list, diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index f391a70..977df85 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -26,14 +26,20 @@ static bool visit_is_output(Visitor *v) return v->type_enum == output_type_enum; } -void visit_start_struct(Visitor *v, const char *name, void **obj, +bool visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { + bool result; + assert(obj ? size : !size); if (obj && visit_is_output(v)) { assert(*obj); } - v->start_struct(v, name, obj, size, errp); + result = v->start_struct(v, name, obj, size, errp); + if (result) { + assert(obj && *obj); + } + return result; } void visit_check_struct(Visitor *v, Error **errp) @@ -48,16 +54,22 @@ void visit_end_struct(Visitor *v) v->end_struct(v); } -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 result = false; + assert(obj && size); if (visit_is_output(v)) { assert(*obj); } if (v->start_implicit_struct) { - v->start_implicit_struct(v, obj, size, errp); + result = v->start_implicit_struct(v, obj, size, errp); } + if (result) { + assert(*obj); + } + return result; } void visit_end_implicit_struct(Visitor *v) @@ -67,10 +79,14 @@ void visit_end_implicit_struct(Visitor *v) } } -void visit_start_list(Visitor *v, const char *name, GenericList **list, +bool visit_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { - v->start_list(v, name, list, errp); + bool result = v->start_list(v, name, list, errp); + if (result) { + assert(list && *list); + } + return result; } GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp) @@ -229,6 +245,7 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) { + Error *err = NULL; assert(obj); /* TODO: Fix callers to not pass NULL when they mean "", so that we * can enable: @@ -237,6 +254,10 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) } */ v->type_str(v, name, obj, errp); + if (!visit_is_output(v) && err) { + *obj = NULL; + } + error_propagate(errp, err); } void visit_type_number(Visitor *v, const char *name, double *obj, @@ -248,11 +269,17 @@ void visit_type_number(Visitor *v, const char *name, double *obj, void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) { + Error *err = NULL; + assert(obj); if (visit_is_output(v)) { assert(*obj); } v->type_any(v, name, obj, errp); + if (!visit_is_output(v) && err) { + *obj = NULL; + } + error_propagate(errp, err); } void visit_type_null(Visitor *v, const char *name, Error **errp) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 550ee93..2427a80 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -130,7 +130,7 @@ static void qmp_input_pop(Visitor *v) qiv->nb_stack--; } -static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, +static bool qmp_input_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); @@ -140,30 +140,34 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "QDict"); - return; + return false; } qmp_input_push(qiv, qobj, NULL, &err); if (err) { error_propagate(errp, err); - return; + return false; } if (obj) { *obj = g_malloc0(size); + return true; } + return false; } -static void qmp_input_start_implicit_struct(Visitor *v, void **obj, +static bool qmp_input_start_implicit_struct(Visitor *v, void **obj, size_t size, Error **errp) { if (obj) { *obj = g_malloc0(size); + return true; } + return false; } -static void qmp_input_start_list(Visitor *v, const char *name, +static bool qmp_input_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); @@ -173,7 +177,7 @@ static void qmp_input_start_list(Visitor *v, const char *name, if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "list"); - return; + return false; } entry = qlist_first(qobject_to_qlist(qobj)); @@ -181,10 +185,12 @@ static void qmp_input_start_list(Visitor *v, const char *name, if (list) { if (entry) { *list = g_new0(GenericList, 1); + return true; } else { *list = NULL; } } + return false; } static GenericList *qmp_input_next_list(Visitor *v, GenericList *list, diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index c9615fe..cd45ffb 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -103,7 +103,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, } } -static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, +static bool qmp_output_start_struct(Visitor *v, const char *name, void **obj, size_t unused, Error **errp) { QmpOutputVisitor *qov = to_qov(v); @@ -111,6 +111,7 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, qmp_output_add(qov, name, dict); qmp_output_push(qov, dict); + return false; } static void qmp_output_end_struct(Visitor *v) @@ -119,7 +120,7 @@ static void qmp_output_end_struct(Visitor *v) qmp_output_pop(qov); } -static void qmp_output_start_list(Visitor *v, const char *name, +static bool qmp_output_start_list(Visitor *v, const char *name, GenericList **listp, Error **errp) { QmpOutputVisitor *qov = to_qov(v); @@ -127,6 +128,7 @@ static void qmp_output_start_list(Visitor *v, const char *name, qmp_output_add(qov, name, list); qmp_output_push(qov, list); + return false; } static GenericList *qmp_output_next_list(Visitor *v, GenericList *list, diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 582a62a..0e4a07c 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -120,7 +120,7 @@ error: siv->ranges = NULL; } -static void +static bool start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { StringInputVisitor *siv = to_siv(v); @@ -132,7 +132,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) parse_str(siv, &err); if (err) { error_propagate(errp, err); - return; + return false; } siv->cur_range = g_list_first(siv->ranges); @@ -142,8 +142,10 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) siv->cur = r->begin; } *list = g_new0(GenericList, 1); + return true; } else { *list = NULL; + return false; } } diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 7209d80..2666802 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -263,7 +263,7 @@ static void print_type_number(Visitor *v, const char *name, double *obj, string_output_set(sov, g_strdup_printf("%f", *obj)); } -static void +static bool start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { StringOutputVisitor *sov = to_sov(v); @@ -276,6 +276,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) if (*list && (*list)->next) { sov->list_mode = LM_STARTED; } + return false; } static GenericList * diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 6016734..eebb5f7 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -58,8 +58,10 @@ def gen_visit_implicit_struct(typ): 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) { goto out; } @@ -69,6 +71,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * visit_type_%(c_type)s_fields(v, obj, &err); out_obj: visit_end_implicit_struct(v); + if (allocated && err) { + g_free(*obj); + *obj = NULL; + } out: error_propagate(errp, err); } @@ -116,18 +122,15 @@ out: 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, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; %(c_name)s *elt; + bool allocated; - visit_start_list(v, name, (GenericList **)obj, &err); + allocated = visit_start_list(v, name, (GenericList **)obj, &err); if (err) { goto out; } @@ -144,6 +147,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } visit_end_list(v); out: + if (allocated && err) { + qapi_free_%(c_name)s(*obj); + *obj = NULL; + } error_propagate(errp, err); } ''', @@ -174,8 +181,10 @@ def gen_visit_alternate(name, variants): void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 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; } @@ -204,11 +213,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } out_obj: visit_end_implicit_struct(v); + 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 @@ -236,8 +249,10 @@ def gen_visit_object(name, base, members, variants): void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; + bool allocated; - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); + allocated = visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), + &err); if (err) { goto out; } @@ -301,10 +316,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error visit_check_struct(v, &err); out_obj: visit_end_struct(v); + 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 bc8085d..d3466a4 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -227,14 +227,13 @@ 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 */ + /* verify that visit_type_XXX() cleans up properly on error */ error_free_or_abort(&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-strict.c b/tests/test-qmp-input-strict.c index 775ad39..4db35dd 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -181,10 +181,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data, visit_type_TestStruct(v, NULL, &p, &err); error_free_or_abort(&err); - if (p) { - g_free(p->string); - } - g_free(p); + g_assert(!p); } static void test_validate_fail_struct_nested(TestInputVisitorData *data, @@ -198,7 +195,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data, visit_type_UserDefTwo(v, NULL, &udp, &err); error_free_or_abort(&err); - qapi_free_UserDefTwo(udp); + g_assert(!udp); } static void test_validate_fail_list(TestInputVisitorData *data, @@ -212,7 +209,7 @@ static void test_validate_fail_list(TestInputVisitorData *data, visit_type_UserDefOneList(v, NULL, &head, &err); error_free_or_abort(&err); - qapi_free_UserDefOneList(head); + g_assert(!head); } static void test_validate_fail_union_native_list(TestInputVisitorData *data, @@ -227,7 +224,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data, visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefNativeListUnion(tmp); + g_assert(!tmp); } static void test_validate_fail_union_flat(TestInputVisitorData *data, @@ -241,7 +238,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data, visit_type_UserDefFlatUnion(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefFlatUnion(tmp); + g_assert(!tmp); } static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, @@ -256,13 +253,13 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefFlatUnion2(tmp); + g_assert(!tmp); } static void test_validate_fail_alternate(TestInputVisitorData *data, const void *unused) { - UserDefAlternate *tmp = NULL; + UserDefAlternate *tmp; Visitor *v; Error *err = NULL; @@ -270,7 +267,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data, visit_type_UserDefAlternate(v, NULL, &tmp, &err); error_free_or_abort(&err); - qapi_free_UserDefAlternate(tmp); + g_assert(!tmp); } static void do_test_validate_qmp_introspect(TestInputVisitorData *data, diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index f6bd408..7f9191c 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -708,18 +708,12 @@ static void test_visitor_in_errors(TestInputVisitorData *data, visit_type_TestStruct(v, NULL, &p, &err); error_free_or_abort(&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_free(p->string); - g_free(p); + g_assert(!p); v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]"); visit_type_strList(v, NULL, &q, &err); error_free_or_abort(&err); - assert(q); - qapi_free_strList(q); + assert(!q); } static void test_visitor_in_wrong_type(TestInputVisitorData *data,
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 pointer-based visit_type_FOO() functions always leave a safe value in *obj during an input visitor (either the new object on success, or NULL if an error is encountered). Since input visitors have blind assignment semantics, we have to track the result of whether an assignment is made all the way down to each visitor callback implementation, to avoid making decisions based on potentially uninitialized storage. Note that we still leave *obj unchanged after a scalar-based visit_type_FOO(); I did not feel like auditing all uses of visit_type_Enum() to see if the callers would tolerate a specific sentinel value (not to mention having to decide whether it would be better to use 0 or ENUM__MAX as that sentinel). Signed-off-by: Eric Blake <eblake@redhat.com> --- v8: rebase to earlier changes v7: rebase to earlier changes, enhance commit message, also fix visit_type_str() and visit_type_any() v6: rebase on top of earlier doc and formatting improvements, mention that *obj can be uninitialized on entry to an input visitor, rework semantics to keep valgrind happy on uninitialized input, break some long lines --- include/qapi/visitor-impl.h | 6 ++--- include/qapi/visitor.h | 53 ++++++++++++++++++++++++++++-------------- qapi/opts-visitor.c | 11 ++++++--- qapi/qapi-dealloc-visitor.c | 9 ++++--- qapi/qapi-visit-core.c | 39 ++++++++++++++++++++++++++----- qapi/qmp-input-visitor.c | 18 +++++++++----- qapi/qmp-output-visitor.c | 6 +++-- qapi/string-input-visitor.c | 6 +++-- qapi/string-output-visitor.c | 3 ++- scripts/qapi-visit.py | 40 +++++++++++++++++++++++-------- tests/test-qmp-commands.c | 13 +++++------ tests/test-qmp-input-strict.c | 19 +++++++-------- tests/test-qmp-input-visitor.c | 10 ++------ 13 files changed, 154 insertions(+), 79 deletions(-)