From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43300) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ1Nf-0001VH-Ch for qemu-devel@nongnu.org; Tue, 12 Jan 2016 11:01:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJ1NZ-0004sI-Bo for qemu-devel@nongnu.org; Tue, 12 Jan 2016 11:01:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38762) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ1NZ-0004ry-4w for qemu-devel@nongnu.org; Tue, 12 Jan 2016 11:01:05 -0500 From: Markus Armbruster References: <20160108091949.GA14724@olga> <20160108130251.GA17847@olga> <20160108143831.GA7632@olga> <1907860725.4.388b58e8-5b06-4844-be0c-df2778eb46fb.open-xchange@webmail.proxmox.com> <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> Date: Tue, 12 Jan 2016 17:00:59 +0100 In-Reply-To: <1786291473.38.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> (Wolfgang Bumiller's message of "Tue, 12 Jan 2016 10:27:43 +0100 (CET)") Message-ID: <87h9iirdms.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wolfgang Bumiller Cc: P J P , Michael Tokarev , qemu-devel@nongnu.org, Ling Liu Wolfgang Bumiller writes: >> On January 12, 2016 at 9:45 AM Markus Armbruster wrote: >> >> Wolfgang Bumiller writes: >> >> > When processing 'sendkey' command, hmp_sendkey routine null >> > terminates the 'keyname_buf' array. This results in an OOB >> >> Well, it technically doesn't terminate, > > It does for combined keys where it replaces the '-' sign (eg. ctrl-alt-f1). > >> > @@ -1749,6 +1749,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) >> > while (1) { >> > separator = strchr(keys, '-'); >> > keyname_len = separator ? separator - keys : strlen(keys); >> > + if (keyname_len >= sizeof(keyname_buf)) >> > + goto err_out; >> >> Style nit: missing braces. >> >> Works because the longest member of QKeyCode_lookup[] is 13 characters, >> and therefore truncation implies "no match". But it's not obvious. >> No worse than before, but "you touch it, you own it". >> >> Options: >> >> * Add a comments >> >> char keyname_buf[16]; /* can hold any QKeyCode */ >> >> and >> >> if (keyname_len >= sizeof(keyname_buf)) { >> /* too long to match any QKeyCode */ >> goto err_out; >> } >> >> * Make index_from_key() take pointer into string and size instead of a >> string. > > That actually looks like a good idea. > >> * Get rid of the static buffer and malloc the sucker already. >> >> > 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"); >> keyname_len = 4; >> } >> keyname_buf[keyname_len] = 0; >> >> Why keep this assignment? > > See above, it terminates keys when using combined keys. You're right. We copy out up to 15 characters, then zap the '-': separator = strchr(keys, '-'); keyname_len = separator ? separator - keys : strlen(keys); pstrcpy(keyname_buf, sizeof(keyname_buf), keys); [...] keyname_buf[keyname_len] = 0; This is stupid. If we already know how many characters we need, we should copy exactly those: separator = strchr(keys, '-'); keyname_len = separator ? separator - keys : strlen(keys); if (keyname_len >= sizeof(keyname_buf)) goto err_out; memcpy(keyname_buf, keyname_len, keys); keyname_buf[keyname_len] = 0; But I'd simply dispense with the static buffer, and do something like separator = strchr(keys, '-'); key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys); [...] g_free(key); This is advice, not recommendation. >> > @@ -1800,7 +1802,7 @@ out: >> > return; >> > >> > err_out: >> > - monitor_printf(mon, "invalid parameter: %s\n", keyname_buf); >> > + monitor_printf(mon, "invalid parameter: %s\n", keys); >> > goto out; >> > } >> >> Before your patch, the message shows the (possibly truncated) offending >> key name. With your patch, it shows the tail of the argument starting >> with the offending key name. Why is that an improvement? > > I guess that's a matter of preference and the if() can be put after the > pstrcpy() without changing the error output. > >> Could use %.*s to print exactly the offending key name. > > Does that work on all supported platforms? (I see windows-related files > in the code base and last time I checked it doesn't do everything.) > If so then this + changing index_from_key() as you suggested above seems > to be the simplest option. git-grep -F '%.*s' shows several instances, at least one of them in Windows code. If we strdup the key, we can simply print it, of course. >> What's wrong with Prasad's simple fix? > > See above, breaks combined keys and eg. 'ctrl-alt-f1' then errors. Will you prepare a revised patch?