From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33834) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHYBd-00088m-UQ for qemu-devel@nongnu.org; Fri, 08 Jan 2016 09:38:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHYBY-0000b0-VR for qemu-devel@nongnu.org; Fri, 08 Jan 2016 09:38:41 -0500 Received: from proxmox.maurer-it.com ([94.136.31.133]:60471) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHYBY-0000an-Pu for qemu-devel@nongnu.org; Fri, 08 Jan 2016 09:38:36 -0500 Date: Fri, 8 Jan 2016 15:38:31 +0100 From: Wolfgang Bumiller Message-ID: <20160108143831.GA7632@olga> References: <20160108091949.GA14724@olga> <20160108130251.GA17847@olga> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: P J P Cc: qemu-devel@nongnu.org, Ling Liu On Fri, Jan 08, 2016 at 07:29:31PM +0530, P J P wrote: > +-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+ > | Ah yes, how could I miss that. Maybe just add a min() around the > | keyname_len computation? > | > | - keyname_len = separator ? separator - keys : strlen(keys); > | + keyname_len = MIN(sizeof(keyname_buf), separator ? separator - keys : strlen(keys)) > > Actually, only use for 'keyname_len' is in the subsequent if statement, And the replacing of the minus in combined keys. > which IIUC compares the keyname_buf for "<" key. Maybe it could say > > + if (!strncmp(keyname_buf, "<-", 2)) > > and remove the 'keyname_len' altogether. This wouldn't catch '<' without '-'. (`sendkey <`) Also, strncmp with a length of 1 (in the original) seems weird. In the end my MIN() approach would be too forgiving when a key name is longer than keyname_buf and its 15-byte substring exists (which I doubt, though). Ie. {15chars}{and more} would be treated as {15chars}. Worse with {15chars}{and more}-{anotherkey}. keyname_len is not useless and perhaps it would be best to just do an early error check there as I do below. This makes sure no OOB write happens and changes the error output to include the 'keys' part instead of keyname_buf. We can do this because the two other `goto err_out` cases happen after using keyname_buf which had been pstrcpy()d from the last keys, so 'keys' will contain the same or more info, and works better for the new case since a string that is too long might be cut off and then only part of it is displayed if the input is say a combination of one too-long (thus invalid) key and a second one. Alternatively the if() can simply happen after pstrcpy() as a cut-off error should be good enough anyway. @@ -1749,6 +1749,9 @@ 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; + pstrcpy(keyname_buf, sizeof(keyname_buf), keys); /* Be compatible with old interface, convert user inputted "<" */ @@ -1800,7 +1803,7 @@ out: return; err_out: - monitor_printf(mon, "invalid parameter: %s\n", keyname_buf); + monitor_printf(mon, "invalid parameter: %s\n", keys); goto out; }