> On January 12, 2016 at 9:45 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> Wolfgang Bumiller <w.bumil...@proxmox.com> 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.
> > @@ -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.
> What's wrong with Prasad's simple fix?
See above, breaks combined keys and eg. 'ctrl-alt-f1' then errors.
