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

[Juan Quintela <quintela@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote:
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> > From: "Daniel P. Berrange" <berrange@redhat.com>
>> 
>> .....
>> 
>> 
>> 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.

ok then.  Thanks for the explanation.

Later, Juan.