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

[Markus Armbruster]

Wolfgang Bumiller <address@hidden> writes:

>> On January 12, 2016 at 9:45 AM Markus Armbruster <address@hidden> wrote:
>> 
>> Wolfgang Bumiller <address@hidden> 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?