From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33216) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WN7zw-0002dV-A8 for qemu-devel@nongnu.org; Mon, 10 Mar 2014 17:44:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WN7zq-0001mf-9a for qemu-devel@nongnu.org; Mon, 10 Mar 2014 17:44:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54374) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WN7zp-0001m6-Tj for qemu-devel@nongnu.org; Mon, 10 Mar 2014 17:44:30 -0400 Message-ID: <531E323A.3000407@redhat.com> Date: Mon, 10 Mar 2014 15:44:26 -0600 From: Eric Blake MIME-Version: 1.0 References: <1394436721-21812-1-git-send-email-cyliu@suse.com> <1394436721-21812-5-git-send-email-cyliu@suse.com> In-Reply-To: <1394436721-21812-5-git-send-email-cyliu@suse.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Q7qNNR6KjRh9ksF5WrAOtisVbaKugrkfl" Subject: Re: [Qemu-devel] [PATCH v22 04/25] improve assertion in qemu_opt_get functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chunyan Liu , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Q7qNNR6KjRh9ksF5WrAOtisVbaKugrkfl Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/10/2014 01:31 AM, Chunyan Liu wrote: > In qemu_opt_set functions, if desc doen't exist but opts_accepts_any is= true, it s/doen't/doesn't/ I mentioned the same problem against v20. It is very depressing when review comments are not addressed. > won't report error, but can still alloc an opt for the option and save = it. > However, after that, when doing qemu_opt_get, this option could be foun= d in opts > but opt->desc is NULL. This is correct, should not be treated as error.= >=20 > This patch would fix vvfat issue after changing to QemuOpts. >=20 > Signed-off-by: Chunyan Liu > --- > util/qemu-option.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) >=20 > diff --git a/util/qemu-option.c b/util/qemu-option.c > index c7639e8..df79235 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -603,7 +603,9 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *= name, bool defval) > } > return defval; > } > - assert(opt->desc && opt->desc->type =3D=3D QEMU_OPT_BOOL); > + if (opt->desc) { > + assert(opt->desc->type =3D=3D QEMU_OPT_BOOL); > + } > return opt->value.boolean; I'm not sure I like this. opt->value is a union, but opt_set() does NOT populate the union when opts_accepts_any() fails. Previously, we were using opt->desc->type as the discriminator for which branch of the union is valid. But with your patch, if an option was set as a string, but then queried as a boolean, we may be reading bogus contents from the union. Or even worse, if someone sets the uint member of the union to 0x100000000 via qemu_opt_set_number(), then later calls qemu_opt_get_bool, the boolean member _might_ read as true on some platforms and false on others, depending on things such as host endiannes= s. How is vvfat broken without this patch? That is, what specific option are you setting without specifying its type, that later triggers the assertion when you try to get the option via a specific type? I'm wondering if the fix should look more like: if (opt->desc) { assert(opt->desc->type =3D=3D QEMU_OPT_BOOL); return opt->value.boolean; } else { code to parse opt->str } so that you are not dereferencing an undefined state of the union. > @@ -625,7 +627,9 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const = char *name, uint64_t defval) > } > return defval; > } > - assert(opt->desc && opt->desc->type =3D=3D QEMU_OPT_NUMBER); > + if (opt->desc) { > + assert(opt->desc->type =3D=3D QEMU_OPT_NUMBER); > + } > return opt->value.uint; > } > =20 > @@ -645,7 +649,9 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const ch= ar *name, uint64_t defval) > } > return defval; > } > - assert(opt->desc && opt->desc->type =3D=3D QEMU_OPT_SIZE); > + if (opt->desc) { > + assert(opt->desc->type =3D=3D QEMU_OPT_SIZE); > + } > return opt->value.uint; Same problem in these two spots. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Q7qNNR6KjRh9ksF5WrAOtisVbaKugrkfl Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTHjI6AAoJEKeha0olJ0Nq4WQIAIKXgM/rihUSvhPhyjyYdhFA N52ho5AMzvRTQXjQ8q3ZxLMq8tNdUGXK7EPqGGiPWANkc85UF6pRja6gWaXBqitR CANCFLaHb2yNL1omog+kHoCu2FElL5jWZLDcuPwWUGvubw0RGHgIe0u+3FtAU8Dm p5sEYISQyNmUDJ9mGVFfH7l/YCQoFH2e0sDmh5VGci+1FUVgpGLSfB7OwkyASpI+ rB1xvNeU/Gm9oRHn6HMo7+6cSbZNcripmQSgW4FbDDwAXphFJsWKstSdSJG2Due2 E2oaJpnpO7x5T0G6+H+G3g8x+VKd5eXBgjpMBTvo/JP8NtWFOQUpgKF5c/gMrgg= =XvyB -----END PGP SIGNATURE----- --Q7qNNR6KjRh9ksF5WrAOtisVbaKugrkfl--