Re: [Qemu-devel] [PATCH v15 02/23] qapi: Guarantee NULL obj on input visitor callback error

[Eric Blake]

On 04/28/2016 07:00 AM, Eric Blake wrote:
>> The commit message lists start_struct(), start_alternate(), next_list(),
>> type_str(), and type_any().  You cover them except for next_list().  Why
>> is that missing?
> 
> Because *obj can be NULL after next_list() if the list is empty.  But
> there may still be a weaker assertion worth having: if err, then *obj
> must be NULL; and if *obj, then err must not be set (weaker in that for
> all the other functions touched, exactly one of the two conditions can
> result, but here, !err and !*obj is valid as a third condition).

Actually, because visit_next_list() can't fail (we removed the errp
argument earlier).  When we finally move the list head allocation into
visit_start_list later in the series (22/23), then we should add the
(weaker) assert there.

> 
> Depending on what else you find later in the series, I may just post a
> fixup for this patch.

Still my plan.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v15 02/23] qapi: Guarantee NULL obj on input visitor callback error

[Eric Blake]

On 04/28/2016 06:24 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Our existing input visitors were not very consistent on errors
>> in a function taking 'TYPE **obj' (that is, start_struct(),
>> start_alternate(), next_list(), type_str(), and type_any()).
>> While all of them set '*obj' to allocated storage on success,
>> it was not obvious whether '*obj' was guaranteed safe on failure,
>> or whether it was left uninitialized.  But a future patch wants
>> to guarantee that visit_type_FOO() does not leak a partially-
>> constructed obj back to the caller; it is easier to implement
>> this if we can reliably state that '*obj' is assigned on exit,
>> even on failures.  Add assertions to enforce it.
> 
> I had to read this several times, because by now I've forgotten that
> we're talking about input visitors only.  Easy enough to avoid: ... that
> input visitors assign to *obj regardless of success or failure.
> 
> Begs the question what is assigned to it on failure, though.

Easy enough to improve the commit message - the intent is that these all
set *obj to NULL on failure.


>> +v->start_struct(v, name, obj, size, );
>> +if (obj && v->type == VISITOR_INPUT) {
>> +assert(err || *obj);
>> +}
>> +error_propagate(errp, err);
>>  }
> 
> The commit message claims you're adding assertions to enforce input
> visitors assign *obj even on failure.  This assertion doesn't do that.
> It enforces "on success, *obj is non-null".  Is that what you want?  Or
> do you actually want something like "either err or *obj are non-null"?
> I.e.
> 
>assert(!err != !*obj);

Hmm - that's an even tighter assertion.  I'll run it through the
testsuite, and if it passes, I'll use it.


> 
> Hmm, you check the postcondition only when v implements
> start_alternate().  Shouldn't it hold regardless of v?  If yes, then
> let's check it regardless of v:
> 
>if (v->start_alternate) {
>v->start_alternate(v, name, obj, size, promote_int, );
>}
>if (v->type == VISITOR_INPUT) {
>assert(err || *obj);
>}
>error_propagate(errp, err);
> 
> But that makes it pretty obvious that the postcondition won't hold when
> !v->start_alternate.  May v->start_alternate() be null for an input
> visitor?  According to visitor-impl.h, it may not.  Okay.

Doesn't hurt to make the check unconditional (to make sure no new input
visitors forget to implement start_alternate).  I'm also debating about
adding an assertion (more likely in the doc patch, since it is less
related to the theme of this one) that obj->type is sensible.


> 
> The commit message lists start_struct(), start_alternate(), next_list(),
> type_str(), and type_any().  You cover them except for next_list().  Why
> is that missing?

Because *obj can be NULL after next_list() if the list is empty.  But
there may still be a weaker assertion worth having: if err, then *obj
must be NULL; and if *obj, then err must not be set (weaker in that for
all the other functions touched, exactly one of the two conditions can
result, but here, !err and !*obj is valid as a third condition).

Depending on what else you find later in the series, I may just post a
fixup for this patch.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v15 02/23] qapi: Guarantee NULL obj on input visitor callback error

[Markus Armbruster]

Eric Blake  writes:

> Our existing input visitors were not very consistent on errors
> in a function taking 'TYPE **obj' (that is, start_struct(),
> start_alternate(), next_list(), type_str(), and type_any()).
> While all of them set '*obj' to allocated storage on success,
> it was not obvious whether '*obj' was guaranteed safe on failure,
> or whether it was left uninitialized.  But a future patch wants
> to guarantee that visit_type_FOO() does not leak a partially-
> constructed obj back to the caller; it is easier to implement
> this if we can reliably state that '*obj' is assigned on exit,
> even on failures.  Add assertions to enforce it.

I had to read this several times, because by now I've forgotten that
we're talking about input visitors only.  Easy enough to avoid: ... that
input visitors assign to *obj regardless of success or failure.

Begs the question what is assigned to it on failure, though.

>
> The opts-visitor start_struct() doesn't set an error, but it
> also was doing a weird check for 0 size; all callers pass in
> non-zero size if obj is non-NULL.
>
> The testsuite has at least one spot where we no longer need
> to pre-initialize a variable prior to a visit; valgrind confirms
> that the test is still fine with the cleanup.
>
> A later patch will document the design constraint implemented
> here.
>
> Signed-off-by: Eric Blake 
>
> ---
> v15: enhance commit message, hoist assertions from later in series
> v14: no change
> v13: no change
> v12: new patch
> ---
>  qapi/qapi-visit-core.c| 34 ++
>  qapi/opts-visitor.c   |  3 ++-
>  qapi/qmp-input-visitor.c  |  4 
>  qapi/string-input-visitor.c   |  1 +
>  tests/test-qmp-input-strict.c |  2 +-
>  5 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 3cd7edc..3a131ce 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -23,7 +23,13 @@
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>  size_t size, Error **errp)
>  {
> -v->start_struct(v, name, obj, size, errp);
> +Error *err = NULL;
> +
> +v->start_struct(v, name, obj, size, );
> +if (obj && v->type == VISITOR_INPUT) {
> +assert(err || *obj);
> +}
> +error_propagate(errp, err);
>  }

The commit message claims you're adding assertions to enforce input
visitors assign *obj even on failure.  This assertion doesn't do that.
It enforces "on success, *obj is non-null".  Is that what you want?  Or
do you actually want something like "either err or *obj are non-null"?
I.e.

   assert(!err != !*obj);

>
>  void visit_end_struct(Visitor *v, Error **errp)
> @@ -51,9 +57,15 @@ void visit_start_alternate(Visitor *v, const char *name,
> GenericAlternate **obj, size_t size,
> bool promote_int, Error **errp)
>  {
> +Error *err = NULL;
> +
>  assert(obj && size >= sizeof(GenericAlternate));
>  if (v->start_alternate) {
> -v->start_alternate(v, name, obj, size, promote_int, errp);
> +v->start_alternate(v, name, obj, size, promote_int, );
> +if (v->type == VISITOR_INPUT) {
> +assert(err || *obj);
> +}
> +error_propagate(errp, err);
>  }
>  }
>

Hmm, you check the postcondition only when v implements
start_alternate().  Shouldn't it hold regardless of v?  If yes, then
let's check it regardless of v:

   if (v->start_alternate) {
   v->start_alternate(v, name, obj, size, promote_int, );
   }
   if (v->type == VISITOR_INPUT) {
   assert(err || *obj);
   }
   error_propagate(errp, err);

But that makes it pretty obvious that the postcondition won't hold when
!v->start_alternate.  May v->start_alternate() be null for an input
visitor?  According to visitor-impl.h, it may not.  Okay.

> @@ -188,7 +200,14 @@ 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)
>  {
> -v->type_str(v, name, obj, errp);
> +Error *err = NULL;
> +
> +assert(obj);
> +v->type_str(v, name, obj, );
> +if (v->type == VISITOR_INPUT) {
> +assert(err || *obj);
> +}
> +error_propagate(errp, err);
>  }
>
>  void visit_type_number(Visitor *v, const char *name, double *obj,
> @@ -199,7 +218,14 @@ void visit_type_number(Visitor *v, const char *name, 
> double *obj,
>
>  void visit_type_any(Visitor *v, const char *name, QObject **obj, Error 
> **errp)
>  {
> -v->type_any(v, name, obj, errp);
> +Error *err = NULL;
> +
> +assert(obj);
> +v->type_any(v, name, obj, );
> +if (v->type == VISITOR_INPUT) {
> +assert(err || *obj);
> +}
> +error_propagate(errp, err);
>  }
>

The commit message lists start_struct(), start_alternate(), next_list(),
type_str(), and 

[Qemu-devel] [PATCH v15 02/23] qapi: Guarantee NULL obj on input visitor callback error

[Eric Blake]

Our existing input visitors were not very consistent on errors
in a function taking 'TYPE **obj' (that is, start_struct(),
start_alternate(), next_list(), type_str(), and type_any()).
While all of them set '*obj' to allocated storage on success,
it was not obvious whether '*obj' was guaranteed safe on failure,
or whether it was left uninitialized.  But a future patch wants
to guarantee that visit_type_FOO() does not leak a partially-
constructed obj back to the caller; it is easier to implement
this if we can reliably state that '*obj' is assigned on exit,
even on failures.  Add assertions to enforce it.

The opts-visitor start_struct() doesn't set an error, but it
also was doing a weird check for 0 size; all callers pass in
non-zero size if obj is non-NULL.

The testsuite has at least one spot where we no longer need
to pre-initialize a variable prior to a visit; valgrind confirms
that the test is still fine with the cleanup.

A later patch will document the design constraint implemented
here.

Signed-off-by: Eric Blake 

---
v15: enhance commit message, hoist assertions from later in series
v14: no change
v13: no change
v12: new patch
---
 qapi/qapi-visit-core.c| 34 ++
 qapi/opts-visitor.c   |  3 ++-
 qapi/qmp-input-visitor.c  |  4 
 qapi/string-input-visitor.c   |  1 +
 tests/test-qmp-input-strict.c |  2 +-
 5 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 3cd7edc..3a131ce 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -23,7 +23,13 @@
 void visit_start_struct(Visitor *v, const char *name, void **obj,
 size_t size, Error **errp)
 {
-v->start_struct(v, name, obj, size, errp);
+Error *err = NULL;
+
+v->start_struct(v, name, obj, size, );
+if (obj && v->type == VISITOR_INPUT) {
+assert(err || *obj);
+}
+error_propagate(errp, err);
 }

 void visit_end_struct(Visitor *v, Error **errp)
@@ -51,9 +57,15 @@ void visit_start_alternate(Visitor *v, const char *name,
GenericAlternate **obj, size_t size,
bool promote_int, Error **errp)
 {
+Error *err = NULL;
+
 assert(obj && size >= sizeof(GenericAlternate));
 if (v->start_alternate) {
-v->start_alternate(v, name, obj, size, promote_int, errp);
+v->start_alternate(v, name, obj, size, promote_int, );
+if (v->type == VISITOR_INPUT) {
+assert(err || *obj);
+}
+error_propagate(errp, err);
 }
 }

@@ -188,7 +200,14 @@ 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)
 {
-v->type_str(v, name, obj, errp);
+Error *err = NULL;
+
+assert(obj);
+v->type_str(v, name, obj, );
+if (v->type == VISITOR_INPUT) {
+assert(err || *obj);
+}
+error_propagate(errp, err);
 }

 void visit_type_number(Visitor *v, const char *name, double *obj,
@@ -199,7 +218,14 @@ void visit_type_number(Visitor *v, const char *name, 
double *obj,

 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
 {
-v->type_any(v, name, obj, errp);
+Error *err = NULL;
+
+assert(obj);
+v->type_any(v, name, obj, );
+if (v->type == VISITOR_INPUT) {
+assert(err || *obj);
+}
+error_propagate(errp, err);
 }

 static void output_type_enum(Visitor *v, const char *name, int *obj,
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 66aeaed..4cb6436 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj,
 const QemuOpt *opt;

 if (obj) {
-*obj = g_malloc0(size > 0 ? size : 1);
+*obj = g_malloc0(size);
 }
 if (ov->depth++ > 0) {
 return;
@@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, 
Error **errp)

 opt = lookup_scalar(ov, name, errp);
 if (!opt) {
+*obj = NULL;
 return;
 }
 *obj = g_strdup(opt->str ? opt->str : "");
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 02d4233..77cce8b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char 
*name, void **obj,
 QObject *qobj = qmp_input_get_object(qiv, name, true);
 Error *err = NULL;

+if (obj) {
+*obj = NULL;
+}
 if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"QDict");
@@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char 
*name, char **obj,
 QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true));

 if (!qstr) {
+*obj = NULL;
 error_setg(errp,