From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHWhX-0005Ds-Uu for qemu-devel@nongnu.org; Fri, 08 Jan 2016 08:03:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHWhS-0001mt-WA for qemu-devel@nongnu.org; Fri, 08 Jan 2016 08:03:31 -0500 Received: from proxmox.maurer-it.com ([94.136.31.133]:57998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHWhS-0001mp-Py for qemu-devel@nongnu.org; Fri, 08 Jan 2016 08:03:26 -0500 Date: Fri, 8 Jan 2016 14:02:51 +0100 From: Wolfgang Bumiller Message-ID: <20160108130251.GA17847@olga> References: <20160108091949.GA14724@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 05:49:51PM +0530, P J P wrote: > Hello, > > +-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+ > | > if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) { > | > pstrcpy(keyname_buf, sizeof(keyname_buf), "less"); > | > - keyname_len = 4; > | > | keyname_buf is a char[16] so 4 will not overflow it. > | > | > } > | > - keyname_buf[keyname_len] = 0; > | > | This last write is also used to separate combined keys, so removing > | this write breaks commands such as `sendkeys ctrl-f1`. > | Better add a -1 to the sizeof()s? > | > | Come to think of it, when is this really an OOB write? > | Given where keyname_len comes from: > | > | | separator = strchr(keys, '-'); > | | keyname_len = separator ? separator - keys : strlen(keys); > > The OOB issue occurs when there is no separator, and strlen(keys) is longer > than '16' characters. In that case, "keyname_buf[keyname_len] = 0;" writes > beyond the 'keyname_buf' array. It's removed because 'pstrcpy()' routine also > null terminates the buffer. 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))