From nobody Fri May 3 14:50:10 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zoho.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1490974115431377.53797429191593; Fri, 31 Mar 2017 08:28:35 -0700 (PDT) Received: from localhost ([::1]:41578 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ctyTZ-0008DL-Sl for importer@patchew.org; Fri, 31 Mar 2017 11:28:33 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54253) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ctySm-0007rf-Tt for qemu-devel@nongnu.org; Fri, 31 Mar 2017 11:27:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ctySl-0003VC-HP for qemu-devel@nongnu.org; Fri, 31 Mar 2017 11:27:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53284) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ctySi-0003ST-6r; Fri, 31 Mar 2017 11:27:40 -0400 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DCC4C8AE72; Fri, 31 Mar 2017 15:27:38 +0000 (UTC) Received: from red.redhat.com (ovpn-122-200.rdu2.redhat.com [10.10.122.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7AD0219C8B; Fri, 31 Mar 2017 15:27:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DCC4C8AE72 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=eblake@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DCC4C8AE72 From: Eric Blake To: qemu-devel@nongnu.org Date: Fri, 31 Mar 2017 10:27:30 -0500 Message-Id: <20170331152730.12514-1-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 31 Mar 2017 15:27:39 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH for-2.9] rbd: Fix regression in legacy key/values containing escaped : X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Josh Durgin , qemu-block@nongnu.org, jcody@redhat.com, armbru@redhat.com, Max Reitz , Alexandru.Avadanii@enea.com Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Commit c7cacb3 accidentally broke legacy key-value parsing through pseudo-filename parsing of -drive file=3Drbd://..., for any key that contains an escaped ':'. Such a key is surprisingly common, thanks to mon_host specifying a 'host:port' string. The break happens because passing things from QDict through QemuOpts back to another QDict requires that we pack our parsed key/value pairs into a string, and then reparse that string, but the intermediate string that we created ("key1=3Dvalue1:key2=3Dvalue2") lost the \: escaping that was present in the original, so that we could no longer see which : were used as separators vs. those used as part of the original input. Fix it by collecting the key/value pairs through a QList, and sending that list on a round trip through a JSON QString (as in '["key1","value1","key2","value2"]') on its way through QemuOpts, rather than hand-rolling our own string. Since the string is only handled internally, this was faster than creating a full-blown struct of '[{"key1":"value1"},{"key2":"value2"}]', and safer at guaranteeing order compared to '{"key1":"value1","key2":"value2"}'. It would be nicer if we didn't have to round-trip through QemuOpts in the first place, but that's a much bigger task for later. Reproducer: ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio \ -drive 'file=3Drbd:volumes/volume-ea141b5c-cdb3-4765-910d-e7008b209a70'\ ':id=3Dcompute:key=3DAQAVkvxXAAAAABAA9ZxWFYdRmV+DSwKr7BKKXg=3D=3D'\ ':auth_supported=3Dcephx\;none:mon_host=3D192.168.1.2\:6789'\ ',format=3Draw,if=3Dnone,id=3Ddrive-virtio-disk0,'\ 'serial=3Dea141b5c-cdb3-4765-910d-e7008b209a70,cache=3Dwriteback' Even without an RBD setup, this serves a test of whether we get the incorrect parser error of: qemu-system-x86_64: -drive file=3Drbd:...cache=3Dwriteback: conf option 678= 9 has no value or the correct behavior of hanging while trying to connect to the requested mon_host of 192.168.1.2:6789. Reported-by: Alexandru Avadanii Signed-off-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: Max Reitz Tested-by: Alexandru Avadanii --- block/rbd.c | 83 +++++++++++++++++++++++++++++++--------------------------= ---- 1 file changed, 42 insertions(+), 41 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index 498322b..fbdb131 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -20,6 +20,7 @@ #include "crypto/secret.h" #include "qemu/cutils.h" #include "qapi/qmp/qstring.h" +#include "qapi/qmp/qjson.h" /* * When specifying the image filename use: @@ -135,18 +136,16 @@ static void qemu_rbd_parse_filename(const char *filen= ame, QDict *options, Error **errp) { const char *start; - char *p, *buf, *keypairs; + char *p, *buf; + QList *keypairs =3D NULL; char *found_str; - size_t max_keypair_size; if (!strstart(filename, "rbd:", &start)) { error_setg(errp, "File name must start with 'rbd:'"); return; } - max_keypair_size =3D strlen(start) + 1; buf =3D g_strdup(start); - keypairs =3D g_malloc0(max_keypair_size); p =3D buf; found_str =3D qemu_rbd_next_tok(p, '/', &p); @@ -194,33 +193,30 @@ static void qemu_rbd_parse_filename(const char *filen= ame, QDict *options, } else if (!strcmp(name, "id")) { qdict_put(options, "user" , qstring_from_str(value)); } else { - /* FIXME: This is pretty ugly, and not the right way to do thi= s. - * These should be contained in a structure, and then - * passed explicitly as individual key/value pairs to - * rados. Consider this legacy code that needs to be - * updated. */ - char *tmp =3D g_malloc0(max_keypair_size); - /* only use a delimiter if it is not the first keypair found */ - /* These are sets of unknown key/value pairs we'll pass along - * to ceph */ - if (keypairs[0]) { - snprintf(tmp, max_keypair_size, ":%s=3D%s", name, value); - pstrcat(keypairs, max_keypair_size, tmp); - } else { - snprintf(keypairs, max_keypair_size, "%s=3D%s", name, valu= e); + /* + * We pass these internally to qemu_rbd_set_keypairs(), so + * we can get away with the simpler list of [ "key1", + * "value1", "key2", "value2" ] rather than a raw dict + * { "key1": "value1", "key2": "value2" } where we can't + * guarantee order, or even a more correct but complex + * [ { "key1": "value1" }, { "key2": "value2" } ] + */ + if (!keypairs) { + keypairs =3D qlist_new(); } - g_free(tmp); + qlist_append(keypairs, qstring_from_str(name)); + qlist_append(keypairs, qstring_from_str(value)); } } - if (keypairs[0]) { - qdict_put(options, "=3Dkeyvalue-pairs", qstring_from_str(keypairs)= ); + if (keypairs) { + qdict_put(options, "=3Dkeyvalue-pairs", + qobject_to_json(QOBJECT(keypairs))); } - done: g_free(buf); - g_free(keypairs); + QDECREF(keypairs); return; } @@ -244,36 +240,41 @@ static int qemu_rbd_set_auth(rados_t cluster, const c= har *secretid, return 0; } -static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs, +static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_jso= n, Error **errp) { - char *p, *buf; - char *name; - char *value; + QList *keypairs; + QString *name; + QString *value; + const char *key; + size_t remaining; int ret =3D 0; - buf =3D g_strdup(keypairs); - p =3D buf; + if (!keypairs_json) { + return ret; + } + keypairs =3D qobject_to_qlist(qobject_from_json(keypairs_json, + &error_abort)); + remaining =3D qlist_size(keypairs) / 2; + assert(remaining); - while (p) { - name =3D qemu_rbd_next_tok(p, '=3D', &p); - if (!p) { - error_setg(errp, "conf option %s has no value", name); - ret =3D -EINVAL; - break; - } + while (remaining--) { + name =3D qobject_to_qstring(qlist_pop(keypairs)); + value =3D qobject_to_qstring(qlist_pop(keypairs)); + assert(name && value); + key =3D qstring_get_str(name); - value =3D qemu_rbd_next_tok(p, ':', &p); - - ret =3D rados_conf_set(cluster, name, value); + ret =3D rados_conf_set(cluster, key, qstring_get_str(value)); + QDECREF(name); + QDECREF(value); if (ret < 0) { - error_setg_errno(errp, -ret, "invalid conf option %s", name); + error_setg_errno(errp, -ret, "invalid conf option %s", key); ret =3D -EINVAL; break; } } - g_free(buf); + QDECREF(keypairs); return ret; } --=20 2.9.3