Re: [Qemu-block] [PATCH 3/6] migration: add support for a "tls-authz" migration parameter

[Daniel P . Berrangé]

On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote:
> Daniel P. Berrangé <address@hidden> wrote:
> > From: "Daniel P. Berrange" <address@hidden>
> 
> .....
> 
> 
> It is not just the fault of this patch, but as you are the one doing the
> tls bits on migration...
> 
> 
> > @@ -1106,6 +1108,12 @@ static void 
> > migrate_params_apply(MigrateSetParameters *params, Error **errp)
> >          s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
> >      }
> >  
> > +    if (params->has_tls_authz) {
> > +        g_free(s->parameters.tls_authz);
> > +        assert(params->tls_authz->type == QTYPE_QSTRING);
> 
> We really try  hard not to use assert() on migration code (yes, it is an
> ongoing effort).  The code around this is something like:
> 
> static void migrate_params_test_apply(MigrateSetParameters *params,
>                                       MigrationParameters *dest)
> {
>     [...]
> 
>     if (params->has_compress_level) {
>         dest->compress_level = params->compress_level;
>     }
> 
>     [...]
> 
>     if (params->has_tls_creds) {
>         assert(params->tls_creds->type == QTYPE_QSTRING);
>         dest->tls_creds = g_strdup(params->tls_creds->u.s);
>     }
>     [...]
> }
> 
> Ok, tls code is the one with strings, but still.

My understanding is that because we declared this parameter to
have "str" type in the QAPI schema, the QAPI code should already
guarantee that  "params->tls_creds->type == QTYPE_QSTRING" is
true.

IOW, the assert should never fail, and if it does, that would
be a good thing as if QAPI wasn't validating input correctly
something very bad has gone wrong.


> static bool migrate_params_check(MigrationParameters *params, Error **errp)
> {
>     if (params->has_compress_level &&
>         (params->compress_level > 9)) {
>         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
>                    "is invalid, it should be in the range of 0 to 9");
>         return false;

This is different because QAPI schema merely declares it to be an
int, so nothing in the QAPI input validation will do range checking.
So this codepath is definitely reachable by users feeding bad input.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|