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