From patchwork Mon Jan 18 14:23:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 8054941 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 08F839F818 for ; Mon, 18 Jan 2016 14:23:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CFB74203A9 for ; Mon, 18 Jan 2016 14:23:24 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BC8EF203A5 for ; Mon, 18 Jan 2016 14:23:23 +0000 (UTC) Received: from localhost ([::1]:60045 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLAiI-0004K9-JX for patchwork-qemu-devel@patchwork.kernel.org; Mon, 18 Jan 2016 09:23:22 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLAiA-0004Jz-OF for qemu-devel@nongnu.org; Mon, 18 Jan 2016 09:23:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLAi5-0005NJ-Ty for qemu-devel@nongnu.org; Mon, 18 Jan 2016 09:23:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58937) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLAi5-0005NF-Lq for qemu-devel@nongnu.org; Mon, 18 Jan 2016 09:23:09 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id CDA4FC049D4F; Mon, 18 Jan 2016 14:23:08 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-113-72.phx2.redhat.com [10.3.113.72]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0IEN5vu013301 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 18 Jan 2016 09:23:07 -0500 Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 85052303F81B; Mon, 18 Jan 2016 15:23:04 +0100 (CET) From: Markus Armbruster To: Wolfgang Bumiller References: <56920EC7.6090109@msgid.tls.msk.ru> <20160111075914.GA28466@olga> <87wprfqj8t.fsf@blackfin.pond.sub.org> <1786291473.38.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> <87h9iirdms.fsf@blackfin.pond.sub.org> <1388889179.80.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> <877fjepwo9.fsf@blackfin.pond.sub.org> <20160113080958.GA18934@olga> <87io2rnir4.fsf@blackfin.pond.sub.org> <20160118133819.GA29453@olga> Date: Mon, 18 Jan 2016 15:23:04 +0100 In-Reply-To: <20160118133819.GA29453@olga> (Wolfgang Bumiller's message of "Mon, 18 Jan 2016 14:38:19 +0100") Message-ID: <87oacjklvb.fsf@blackfin.pond.sub.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Ling Liu , Michael Tokarev , qemu-devel@nongnu.org, P J P Subject: Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Wolfgang Bumiller writes: > On Mon, Jan 18, 2016 at 02:02:07PM +0100, Markus Armbruster wrote: >> Wolfgang Bumiller writes: >> >> > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote: >> >> Wolfgang Bumiller writes: >> >> >> > while (1) { >> > separator = strchr(keys, '-'); >> > keyname_len = separator ? separator - keys : strlen(keys); >> >> Preexisting: I wonder why the compiler doesn't warn here: separator - >> keys is ptrdiff_t, strlen() is size_t, and the left hand side is int. > > I noticed and agree it should warn. We know that separator > keys (ie > positive), but we also use keyname_len as a '.*' parameter to printf() > which expects it to be an 'int', so when changing it to size_t we need > to cast it there. Would have to pass a pretty long key name for this to > be an issue... can this happen over any sane interface that doesn't > already give you the power to just 'kill -9 $qemu'? Merely unclean, not actually broken in a practical sense. >> > - pstrcpy(keyname_buf, sizeof(keyname_buf), keys); >> > >> > /* Be compatible with old interface, convert user inputted "<" */ >> > - if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) { >> > - pstrcpy(keyname_buf, sizeof(keyname_buf), "less"); >> > + if (!strncmp(keys, "<", 1) && keyname_len == 1) { >> >> This strncmp() is a rather roundabout way to say keys[0] == '<'. I >> guess I'd dumb it down while touching it. Your choice. > > Yes, but with the previous pstrcpy() of "less" etc. I thought this was a > style thing (and the compiler optimizes it out anyway last time I > checked). > >> > + keys = "less"; >> >> Works because we're resetting keys to point into the argument string at >> the end of the loop. >> >> > keyname_len = 4; >> > } >> > - keyname_buf[keyname_len] = 0; >> > >> > keylist = g_malloc0(sizeof(*keylist)); >> > keylist->value = g_malloc0(sizeof(*keylist->value)); >> > @@ -1769,16 +1766,16 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) >> > } >> > tmp = keylist; >> > >> > - if (strstart(keyname_buf, "0x", NULL)) { >> > + if (strstart(keys, "0x", NULL)) { >> > char *endp; >> > - int value = strtoul(keyname_buf, &endp, 0); >> > - if (*endp != '\0') { >> > + int value = strtoul(keys, &endp, 0); >> > + if (*endp != '\0' && *endp != '-') { >> >> strtoul() will not parse beyond keyname_len, because it'll only accept >> hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop. >> >> I guess I'd throw in assert(endp <= keys + keyname_len), and test >> endp != keys + keyname_len. What do you think? > > Makes sense, but I doubt it'll ever be hit with sane strtoul() > implementations, but an assetion can't be harmful here either :-) The assertion primarily documents we've considered strtoul() reading beyond the bound. It might also protects us from hasty, incorrect changes in the future, but I guess that's secondary in this case. Preexisting: we don't check errno. Out of scope for this patch. >> > goto err_out; >> > } >> > keylist->value->type = KEY_VALUE_KIND_NUMBER; >> > keylist->value->u.number = value; >> > } else { >> > - int idx = index_from_key(keyname_buf); >> > + int idx = index_from_key(keys, keyname_len); >> > if (idx == Q_KEY_CODE__MAX) { >> > goto err_out; >> > } >> > @@ -1800,7 +1797,7 @@ out: >> > return; >> > >> > err_out: >> > - monitor_printf(mon, "invalid parameter: %s\n", keyname_buf); >> > + monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys); >> > goto out; >> > } >> > >> > diff --git a/include/ui/console.h b/include/ui/console.h >> > index adac36d..116bc2b 100644 >> > --- a/include/ui/console.h >> > +++ b/include/ui/console.h >> > @@ -448,7 +448,7 @@ static inline int vnc_display_pw_expire(const char *id, time_t expires) >> > void curses_display_init(DisplayState *ds, int full_screen); >> > >> > /* input.c */ >> > -int index_from_key(const char *key); >> > +int index_from_key(const char *key, size_t key_length); >> > >> > /* gtk.c */ >> > void early_gtk_display_init(int opengl); >> > diff --git a/ui/input-legacy.c b/ui/input-legacy.c >> > index 35dfc27..3454055 100644 >> > --- a/ui/input-legacy.c >> > +++ b/ui/input-legacy.c >> > @@ -57,12 +57,13 @@ struct QEMUPutLEDEntry { >> > static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = >> > QTAILQ_HEAD_INITIALIZER(led_handlers); >> > >> > -int index_from_key(const char *key) >> > +int index_from_key(const char *key, size_t key_length) >> > { >> > int i; >> > >> > for (i = 0; QKeyCode_lookup[i] != NULL; i++) { >> > - if (!strcmp(key, QKeyCode_lookup[i])) { >> > + if (!strncmp(key, QKeyCode_lookup[i], key_length) && >> > + !QKeyCode_lookup[i][key_length]) { >> > break; >> > } >> > } >> >> Could !strncmp(key, QKeyCode_lookup[i], key_length + 1), but that's >> probably overly clever. > > That's assuming the key name ends with a \0, which is not the case > coming from a combined key combination where key points to "ctrl-alt-f1" > and should find "ctrl". You're right. >> Overall, this is more subtle than a simple g_strndup() solution. But it >> doesn't quite reach the threshold for me asking you to redo it >> differently. >> >> I can work in the two changes I proposed on commit, if you like them: >> dumb down the test for "<", and add the assertion. > > Sounds good. Applied to my monitor-next with these tweaks: diff --git a/hmp.c b/hmp.c index 8be03df..9c571f5 100644 --- a/hmp.c +++ b/hmp.c @@ -1739,7 +1739,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) keyname_len = separator ? separator - keys : strlen(keys); /* Be compatible with old interface, convert user inputted "<" */ - if (!strncmp(keys, "<", 1) && keyname_len == 1) { + if (keys[0] == '<' && keyname_len == 1) { keys = "less"; keyname_len = 4; } @@ -1758,7 +1758,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) if (strstart(keys, "0x", NULL)) { char *endp; int value = strtoul(keys, &endp, 0); - if (*endp != '\0' && *endp != '-') { + assert(endp <= keys + keyname_len); + if (endp != keys + keyname_len) { goto err_out; } keylist->value->type = KEY_VALUE_KIND_NUMBER;