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

[Wolfgang Bumiller]

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

> > @@ -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.