Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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))