From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fVa5Q-0004uz-UU for qemu-devel@nongnu.org; Wed, 20 Jun 2018 06:11:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fVa5P-0003mK-Rr for qemu-devel@nongnu.org; Wed, 20 Jun 2018 06:11:36 -0400 From: Juan Quintela In-Reply-To: <20180620100755.GH3441@redhat.com> ("Daniel P. =?utf-8?Q?Berr?= =?utf-8?Q?ang=C3=A9=22's?= message of "Wed, 20 Jun 2018 11:07:55 +0100") References: <20180615155103.11924-1-berrange@redhat.com> <20180615155103.11924-4-berrange@redhat.com> <87efh1lqpq.fsf@secure.mitica> <20180620100755.GH3441@redhat.com> Reply-To: quintela@redhat.com Date: Wed, 20 Jun 2018 12:11:27 +0200 Message-ID: <87a7rplqcw.fsf@secure.mitica> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/6] migration: add support for a "tls-authz" migration parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. =?utf-8?Q?Berrang=C3=A9?=" Cc: qemu-devel@nongnu.org, Eric Blake , Kevin Wolf , Max Reitz , Markus Armbruster , Gerd Hoffmann , =?utf-8?Q?Marc-?= =?utf-8?Q?Andr=C3=A9?= Lureau , qemu-block@nongnu.org, "Dr. David Alan Gilbert" , Paolo Bonzini Daniel P. Berrang=C3=A9 wrote: > On Wed, Jun 20, 2018 at 12:03:45PM +0200, Juan Quintela wrote: >> Daniel P. Berrang=C3=A9 wrote: >> > From: "Daniel P. Berrange" >>=20 >> ..... >>=20 >>=20 >> It is not just the fault of this patch, but as you are the one doing the >> tls bits on migration... >>=20 >>=20 >> > @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetPara= meters *params, Error **errp) >> > s->parameters.tls_hostname =3D g_strdup(params->tls_hostname-= >u.s); >> > } >> >=20=20 >> > + if (params->has_tls_authz) { >> > + g_free(s->parameters.tls_authz); >> > + assert(params->tls_authz->type =3D=3D QTYPE_QSTRING); >>=20 >> We really try hard not to use assert() on migration code (yes, it is an >> ongoing effort). The code around this is something like: >>=20 >> static void migrate_params_test_apply(MigrateSetParameters *params, >> MigrationParameters *dest) >> { >> [...] >>=20 >> if (params->has_compress_level) { >> dest->compress_level =3D params->compress_level; >> } >>=20 >> [...] >>=20 >> if (params->has_tls_creds) { >> assert(params->tls_creds->type =3D=3D QTYPE_QSTRING); >> dest->tls_creds =3D g_strdup(params->tls_creds->u.s); >> } >> [...] >> } >>=20 >> 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 =3D=3D 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 **er= rp) >> { >> 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.