[Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Pawit Pornkitprasan]

The qmp_migrate method uses the 'blk' and 'inc' parameter without
checking if they're valid or not (they may be uninitialized if
command is received via QMP)

Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
---
 migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration.c b/migration.c
index 9f5a423..f3d1ff7 100644
--- a/migration.c
+++ b/migration.c
@@ -385,8 +385,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationParams params;
     const char *p;
 
-    params.blk = blk;
-    params.shared = inc;
+    params.blk = has_blk && blk;
+    params.shared = has_inc && inc;
 
     if (s->state == MIG_STATE_ACTIVE) {
         error_set(errp, QERR_MIGRATION_ACTIVE);
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On 07/18/2013 08:36 PM, Pawit Pornkitprasan wrote:
> The qmp_migrate method uses the 'blk' and 'inc' parameter without
> checking if they're valid or not (they may be uninitialized if
> command is received via QMP)
> 
> Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
> ---
>  migration.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

However, wouldn't it be nice if we improved the qapi generator to
guarantee a sane default value for optional parameters, even when
has_value is false?

> 
> diff --git a/migration.c b/migration.c
> index 9f5a423..f3d1ff7 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -385,8 +385,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = blk;
> -    params.shared = inc;
> +    params.blk = has_blk && blk;
> +    params.shared = has_inc && inc;
>  
>      if (s->state == MIG_STATE_ACTIVE) {
>          error_set(errp, QERR_MIGRATION_ACTIVE);
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Pawit Pornkitprasan]

On Fri, Jul 19, 2013 at 7:57 PM, Eric Blake <eblake@redhat.com> wrote:
>
> However, wouldn't it be nice if we improved the qapi generator to
> guarantee a sane default value for optional parameters, even when
> has_value is false?
>

I'm not a qemu expert or anything, but wouldn't that destroy the point
of has_value in the first place?

--
Pawit

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On 07/19/2013 05:04 AM, Pawit Pornkitprasan wrote:
> On Fri, Jul 19, 2013 at 7:57 PM, Eric Blake <eblake@redhat.com> wrote:
>>
>> However, wouldn't it be nice if we improved the qapi generator to
>> guarantee a sane default value for optional parameters, even when
>> has_value is false?
>>
> 
> I'm not a qemu expert or anything, but wouldn't that destroy the point
> of has_value in the first place?

There are two uses for has_value.  One: on output, the user knows
whether the variable has been set to anything sane.  Two: on input, the
receiver knows if the caller passed an optional value.  For output,
has_value is always needed.  But for input, we could probably simplify a
LOT of code if 'value' already had a sane default of 0, without having
to first check 'has_value', instead of the current state where 'value'
must be treated as uninitialized if 'has_value' is false.  Improving the
qapi generator to guarantee 0 value of optional parameters that were not
specified in JSON should not block your patch (it would be a series by
itself), it's just a question of whether it is worth making the change
and the resulting simplifications we can make to code that can assume
sane default values.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Luiz Capitulino]

On Fri, 19 Jul 2013 04:57:51 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/18/2013 08:36 PM, Pawit Pornkitprasan wrote:
> > The qmp_migrate method uses the 'blk' and 'inc' parameter without
> > checking if they're valid or not (they may be uninitialized if
> > command is received via QMP)
> > 
> > Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
> > ---
> >  migration.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> However, wouldn't it be nice if we improved the qapi generator to
> guarantee a sane default value for optional parameters, even when
> has_value is false?

We could do that for bool and pointers, but this wouldn't help
integers and enums. Also, even if we had default values, I guess
I'd enforce a common idiom for handling optionals as this is also
a good practice for preventing bugs.

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 19 Jul 2013 04:57:51 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
>> On 07/18/2013 08:36 PM, Pawit Pornkitprasan wrote:
>> > The qmp_migrate method uses the 'blk' and 'inc' parameter without
>> > checking if they're valid or not (they may be uninitialized if
>> > command is received via QMP)
>> > 
>> > Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
>> > ---
>> >  migration.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>> However, wouldn't it be nice if we improved the qapi generator to
>> guarantee a sane default value for optional parameters, even when
>> has_value is false?
>
> We could do that for bool and pointers, but this wouldn't help
> integers and enums. Also, even if we had default values, I guess
> I'd enforce a common idiom for handling optionals as this is also
> a good practice for preventing bugs.

I disagree on pointers.

"have_ptr && ptr->..." is a stupid idiom.  The common idiom for safe
dereference is "ptr && ptr->...".

"bool have_ptr, FOO *ptr" in a parameter list is unidiomatic C.
Idiomatic is just "FOO *ptr".

Conciseness is a virtue.

For non-pointers, there's no special "don't have one" value, so we'd
have to declare a default value in the schema to get rid of the
have_FOO.

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Anthony Liguori]

On Mon, Jul 29, 2013 at 7:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> On Fri, 19 Jul 2013 04:57:51 -0600
>> Eric Blake <eblake@redhat.com> wrote:
>>
>>> On 07/18/2013 08:36 PM, Pawit Pornkitprasan wrote:
>>> > The qmp_migrate method uses the 'blk' and 'inc' parameter without
>>> > checking if they're valid or not (they may be uninitialized if
>>> > command is received via QMP)
>>> >
>>> > Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
>>> > ---
>>> >  migration.c | 4 ++--
>>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>> However, wouldn't it be nice if we improved the qapi generator to
>>> guarantee a sane default value for optional parameters, even when
>>> has_value is false?
>>
>> We could do that for bool and pointers, but this wouldn't help
>> integers and enums. Also, even if we had default values, I guess
>> I'd enforce a common idiom for handling optionals as this is also
>> a good practice for preventing bugs.
>
> I disagree on pointers.
>
> "have_ptr && ptr->..." is a stupid idiom.  The common idiom for safe
> dereference is "ptr && ptr->...".
>
> "bool have_ptr, FOO *ptr" in a parameter list is unidiomatic C.
> Idiomatic is just "FOO *ptr".
>
> Conciseness is a virtue.
>
> For non-pointers, there's no special "don't have one" value, so we'd
> have to declare a default value in the schema to get rid of the
> have_FOO.

The problem is the protocol.  There are cases where the absence of a
value is different than having a default value for the parameter.

This was part of the discussion way back when this all was first
introduced.  Since everything was open coded and we had to preserve
the semantics, that was the only choice we had.

Regards.

Anthony Liguori

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On Mon, Jul 29, 2013 at 7:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>
>>> On Fri, 19 Jul 2013 04:57:51 -0600
>>> Eric Blake <eblake@redhat.com> wrote:
>>>
>>>> On 07/18/2013 08:36 PM, Pawit Pornkitprasan wrote:
>>>> > The qmp_migrate method uses the 'blk' and 'inc' parameter without
>>>> > checking if they're valid or not (they may be uninitialized if
>>>> > command is received via QMP)
>>>> >
>>>> > Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
>>>> > ---
>>>> >  migration.c | 4 ++--
>>>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>
>>>> However, wouldn't it be nice if we improved the qapi generator to
>>>> guarantee a sane default value for optional parameters, even when
>>>> has_value is false?
>>>
>>> We could do that for bool and pointers, but this wouldn't help
>>> integers and enums. Also, even if we had default values, I guess
>>> I'd enforce a common idiom for handling optionals as this is also
>>> a good practice for preventing bugs.
>>
>> I disagree on pointers.
>>
>> "have_ptr && ptr->..." is a stupid idiom.  The common idiom for safe
>> dereference is "ptr && ptr->...".
>>
>> "bool have_ptr, FOO *ptr" in a parameter list is unidiomatic C.
>> Idiomatic is just "FOO *ptr".
>>
>> Conciseness is a virtue.
>>
>> For non-pointers, there's no special "don't have one" value, so we'd
>> have to declare a default value in the schema to get rid of the
>> have_FOO.
>
> The problem is the protocol.  There are cases where the absence of a
> value is different than having a default value for the parameter.
>
> This was part of the discussion way back when this all was first
> introduced.  Since everything was open coded and we had to preserve
> the semantics, that was the only choice we had.

Yes, there is a difference between an optional parameter with a default
value and one without.

If you got a value QMP clients cannot send, the difference can be
eliminated: just pass this value to stand for "nothing".  This is the
case for pointers: just pass NULL.  Like everybody else does when
passing "nothing".

When "nothing" is to be interpreted just like a context-independent
default value, then the difference doesn't matter.  The client doesn't
care, and the function handling the message becomes simpler.

Only when "nothing" can mean different things do you really need both
have_FOO and FOO parameters.

I can't see why we couldn't introduce default values into the schema if
we wanted, and use them to simplify generated code.

For pointers, we don't even need schema extensions to simplify away the
silly have_FOOs.

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Anthony Liguori]

On Tue, Jul 30, 2013 at 1:39 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
>
>> This was part of the discussion way back when this all was first
>> introduced.  Since everything was open coded and we had to preserve
>> the semantics, that was the only choice we had.
>
> Yes, there is a difference between an optional parameter with a default
> value and one without.
>
> If you got a value QMP clients cannot send, the difference can be
> eliminated: just pass this value to stand for "nothing".  This is the
> case for pointers: just pass NULL.  Like everybody else does when
> passing "nothing".
>
> When "nothing" is to be interpreted just like a context-independent
> default value, then the difference doesn't matter.  The client doesn't
> care, and the function handling the message becomes simpler.
>
> Only when "nothing" can mean different things do you really need both
> have_FOO and FOO parameters.

Right, and I'm saying that we have cases like this (especially with
bools).  Unfortunately, there are places in the protocol that have
tristate values for bool parameters and the behavior is distinctly
different for false, true, and not-present.

So yes, we could special case those bits and use sentinel values for
everything else.  Just realize that we need to handle the above case.

> I can't see why we couldn't introduce default values into the schema if
> we wanted, and use them to simplify generated code.
>
> For pointers, we don't even need schema extensions to simplify away the
> silly have_FOOs.

Except you have to carefully audit all users to make sure of that.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On Tue, Jul 30, 2013 at 1:39 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>
>>> This was part of the discussion way back when this all was first
>>> introduced.  Since everything was open coded and we had to preserve
>>> the semantics, that was the only choice we had.
>>
>> Yes, there is a difference between an optional parameter with a default
>> value and one without.
>>
>> If you got a value QMP clients cannot send, the difference can be
>> eliminated: just pass this value to stand for "nothing".  This is the
>> case for pointers: just pass NULL.  Like everybody else does when
>> passing "nothing".
>>
>> When "nothing" is to be interpreted just like a context-independent
>> default value, then the difference doesn't matter.  The client doesn't
>> care, and the function handling the message becomes simpler.
>>
>> Only when "nothing" can mean different things do you really need both
>> have_FOO and FOO parameters.
>
> Right, and I'm saying that we have cases like this (especially with
> bools).  Unfortunately, there are places in the protocol that have

No argument there.

> tristate values for bool parameters and the behavior is distinctly
> different for false, true, and not-present.
>
> So yes, we could special case those bits and use sentinel values for
> everything else.  Just realize that we need to handle the above case.

Yes, we do, and it's understandable that only the most general case got
implemented.

>> I can't see why we couldn't introduce default values into the schema if
>> we wanted, and use them to simplify generated code.
>>
>> For pointers, we don't even need schema extensions to simplify away the
>> silly have_FOOs.
>
> Except you have to carefully audit all users to make sure of that.

For pointers, the audit should be trivial: change generator to omit the
have_FOO, compile, fix up the compile errors (drop have_FOO arguments,
replace have_FOO tests be FOO tests), done.

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Orit Wasserman]

On 07/19/2013 05:36 AM, Pawit Pornkitprasan wrote:
> The qmp_migrate method uses the 'blk' and 'inc' parameter without
> checking if they're valid or not (they may be uninitialized if
> command is received via QMP)
> 
> Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
> ---
>  migration.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 9f5a423..f3d1ff7 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -385,8 +385,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = blk;
> -    params.shared = inc;
> +    params.blk = has_blk && blk;
> +    params.shared = has_inc && inc;
>  
>      if (s->state == MIG_STATE_ACTIVE) {
>          error_set(errp, QERR_MIGRATION_ACTIVE);
> 

Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Luiz Capitulino]

On Fri, 19 Jul 2013 11:36:41 +0900
Pawit Pornkitprasan <p.pawit@gmail.com> wrote:

> The qmp_migrate method uses the 'blk' and 'inc' parameter without
> checking if they're valid or not (they may be uninitialized if
> command is received via QMP)
> 
> Signed-off-by: Pawit Pornkitprasan <p.pawit@gmail.com>
> ---
>  migration.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 9f5a423..f3d1ff7 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -385,8 +385,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationParams params;
>      const char *p;
>  
> -    params.blk = blk;
> -    params.shared = inc;
> +    params.blk = has_blk && blk;
> +    params.shared = has_inc && inc;

This doesn't apply anymore, can you rebase and resend?

By grepping around I can see that there are several instances of
this bug in other commands. I'm surprised we never got a single
bug report about this... Would you mind to fix all the instances?

>  
>      if (s->state == MIG_STATE_ACTIVE) {
>          error_set(errp, QERR_MIGRATION_ACTIVE);

Re: [Qemu-devel] [PATCH] migration: don't use uninitialized variables

[Pawit Pornkitprasan]

On Tue, Jul 30, 2013 at 5:52 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
> This doesn't apply anymore, can you rebase and resend?
>

Sure.

> By grepping around I can see that there are several instances of
> this bug in other commands. I'm surprised we never got a single
> bug report about this... Would you mind to fix all the instances?
>

It's a bug I've found during my internship. As my internship is ending,
I won't be able to fix and test the other instances. Sorry about that.