From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39812) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adEk2-0005vr-0h for qemu-devel@nongnu.org; Tue, 08 Mar 2016 05:19:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adEjy-0003wF-Pu for qemu-devel@nongnu.org; Tue, 08 Mar 2016 05:19:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59212) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adEjy-0003w9-IR for qemu-devel@nongnu.org; Tue, 08 Mar 2016 05:19:46 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 38F316C9 for ; Tue, 8 Mar 2016 10:19:46 +0000 (UTC) Date: Tue, 8 Mar 2016 11:19:44 +0100 From: Kevin Wolf Message-ID: <20160308101944.GD5807@noname.str.redhat.com> References: <1457420446-25276-1-git-send-email-peterx@redhat.com> <1457420446-25276-2-git-send-email-peterx@redhat.com> <87pov5e5w3.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87pov5e5w3.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, Peter Xu , Luiz Capitulino Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben: > Cc: Kevin, because he added the array in question. > > Peter Xu writes: > > > Suggested-by: Paolo Bonzini > > CC: Luiz Capitulino > > Signed-off-by: Peter Xu > > --- > > qobject/qdict.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/qobject/qdict.c b/qobject/qdict.c > > index 9833bd0..eb602a7 100644 > > --- a/qobject/qdict.c > > +++ b/qobject/qdict.c > > @@ -704,17 +704,19 @@ int qdict_array_entries(QDict *src, const char *subqdict) > > for (i = 0; i < INT_MAX; i++) { > > QObject *subqobj; > > int subqdict_entries; > > - size_t slen = 32 + subqdict_len; > > - char indexstr[slen], prefix[slen]; > > +#define __SLEN_MAX (128) > > + char indexstr[__SLEN_MAX], prefix[__SLEN_MAX]; > > size_t snprintf_ret; > > > > - snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i); > > - assert(snprintf_ret < slen); > > + assert(__SLEN_MAX >= 32 + subqdict_len); > > + > > + snprintf_ret = snprintf(indexstr, __SLEN_MAX, "%s%u", subqdict, i); > > + assert(snprintf_ret < __SLEN_MAX); > > > > subqobj = qdict_get(src, indexstr); > > > > - snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i); > > - assert(snprintf_ret < slen); > > + snprintf_ret = snprintf(prefix, __SLEN_MAX, "%s%u.", subqdict, i); > > + assert(snprintf_ret < __SLEN_MAX); > > > > subqdict_entries = qdict_count_prefixed_entries(src, prefix); > > if (subqdict_entries < 0) { > > @@ -745,6 +747,7 @@ int qdict_array_entries(QDict *src, const char *subqdict) > > } > > > > return i; > > +#undef __SLEN_MAX > > } > > > > /** > > Same arguments as for PATCH 2, except here an argument on the maximum > length of subqdict would probably be easier. Yes, these are constant string literals in all callers, including the one non-test case in quorum. Let's simply assert a reasonable maximum for subqdict_length. The minimum we need to allow with the existing callers is 9, and I expect we'll never get keys longer than 16 characters. > Unrelated to your patch: I think we've pushed QDict use father than > sensible. Encoding multiple keys in a string so you can use a flat > associative array as your catch-all data structure is appropriate in > AWK, but in C? Not so much... We'll always to that, because it's the command line syntax. What you may criticise is that we convert QAPI objects to the command line representation instead of the other way around, but changing that (which I think would be far from trivial, for relatively little use) wouldn't get rid of this kind of key parsing, but just move it a bit closer to the command line handling. Kevin