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