Wolfgang Bumiller <w.bumil...@proxmox.com> writes:
>> 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.
You're right. We copy out up to 15 characters, then zap the '-':
separator = strchr(keys, '-');
keyname_len = separator ? separator - keys : strlen(keys);
pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
[...]
keyname_buf[keyname_len] = 0;
This is stupid. If we already know how many characters we need, we
should copy exactly those:
separator = strchr(keys, '-');
keyname_len = separator ? separator - keys : strlen(keys);
if (keyname_len >= sizeof(keyname_buf))
goto err_out;
memcpy(keyname_buf, keyname_len, keys);
keyname_buf[keyname_len] = 0;
But I'd simply dispense with the static buffer, and do something like
separator = strchr(keys, '-');
key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys);
[...]
g_free(key);
This is advice, not recommendation.
>> > @@ -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.
git-grep -F '%.*s' shows several instances, at least one of them in
Windows code.
If we strdup the key, we can simply print it, of course.
>> What's wrong with Prasad's simple fix?
>
> See above, breaks combined keys and eg. 'ctrl-alt-f1' then errors.
Will you prepare a revised patch?
