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

[Wolfgang Bumiller]

On Mon, Jan 18, 2016 at 02:02:07PM +0100, Markus Armbruster wrote:
> Wolfgang Bumiller <address@hidden> writes:
> 
> > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote:
> >> Wolfgang Bumiller <address@hidden> writes:
> >> 
> >      while (1) {
> >          separator = strchr(keys, '-');
> >          keyname_len = separator ? separator - keys : strlen(keys);
> 
> Preexisting: I wonder why the compiler doesn't warn here: separator -
> keys is ptrdiff_t, strlen() is size_t, and the left hand side is int.

I noticed and agree it should warn. We know that separator > keys (ie
positive), but we also use keyname_len as a '.*' parameter to printf()
which expects it to be an 'int', so when changing it to size_t we need
to cast it there. Would have to pass a pretty long key name for this to
be an issue... can this happen over any sane interface that doesn't
already give you the power to just 'kill -9 $qemu'?

> > -        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");
> > +        if (!strncmp(keys, "<", 1) && keyname_len == 1) {
> 
> This strncmp() is a rather roundabout way to say keys[0] == '<'.  I
> guess I'd dumb it down while touching it.  Your choice.

Yes, but with the previous pstrcpy() of "less" etc. I thought this was a
style thing (and the compiler optimizes it out anyway last time I
checked).

> > +            keys = "less";
> 
> Works because we're resetting keys to point into the argument string at
> the end of the loop.
> 
> >              keyname_len = 4;
> >          }
> > -        keyname_buf[keyname_len] = 0;
> >  
> >          keylist = g_malloc0(sizeof(*keylist));
> >          keylist->value = g_malloc0(sizeof(*keylist->value));
> > @@ -1769,16 +1766,16 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
> >          }
> >          tmp = keylist;
> >  
> > -        if (strstart(keyname_buf, "0x", NULL)) {
> > +        if (strstart(keys, "0x", NULL)) {
> >              char *endp;
> > -            int value = strtoul(keyname_buf, &endp, 0);
> > -            if (*endp != '\0') {
> > +            int value = strtoul(keys, &endp, 0);
> > +            if (*endp != '\0' && *endp != '-') {
> 
> strtoul() will not parse beyond keyname_len, because it'll only accept
> hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop.
> 
> I guess I'd throw in assert(endp <= keys + keyname_len), and test
> endp != keys + keyname_len.  What do you think?

Makes sense, but I doubt it'll ever be hit with sane strtoul()
implementations, but an assetion can't be harmful here either :-)

> >                  goto err_out;
> >              }
> >              keylist->value->type = KEY_VALUE_KIND_NUMBER;
> >              keylist->value->u.number = value;
> >          } else {
> > -            int idx = index_from_key(keyname_buf);
> > +            int idx = index_from_key(keys, keyname_len);
> >              if (idx == Q_KEY_CODE__MAX) {
> >                  goto err_out;
> >              }
> > @@ -1800,7 +1797,7 @@ out:
> >      return;
> >  
> >  err_out:
> > -    monitor_printf(mon, "invalid parameter: %s\n", keyname_buf);
> > +    monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys);
> >      goto out;
> >  }
> >  
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index adac36d..116bc2b 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -448,7 +448,7 @@ static inline int vnc_display_pw_expire(const char *id, 
> > time_t expires)
> >  void curses_display_init(DisplayState *ds, int full_screen);
> >  
> >  /* input.c */
> > -int index_from_key(const char *key);
> > +int index_from_key(const char *key, size_t key_length);
> >  
> >  /* gtk.c */
> >  void early_gtk_display_init(int opengl);
> > diff --git a/ui/input-legacy.c b/ui/input-legacy.c
> > index 35dfc27..3454055 100644
> > --- a/ui/input-legacy.c
> > +++ b/ui/input-legacy.c
> > @@ -57,12 +57,13 @@ struct QEMUPutLEDEntry {
> >  static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers =
> >      QTAILQ_HEAD_INITIALIZER(led_handlers);
> >  
> > -int index_from_key(const char *key)
> > +int index_from_key(const char *key, size_t key_length)
> >  {
> >      int i;
> >  
> >      for (i = 0; QKeyCode_lookup[i] != NULL; i++) {
> > -        if (!strcmp(key, QKeyCode_lookup[i])) {
> > +        if (!strncmp(key, QKeyCode_lookup[i], key_length) &&
> > +            !QKeyCode_lookup[i][key_length]) {
> >              break;
> >          }
> >      }
> 
> Could !strncmp(key, QKeyCode_lookup[i], key_length + 1), but that's
> probably overly clever.

That's assuming the key name ends with a \0, which is not the case
coming from a combined key combination where key points to "ctrl-alt-f1"
and should find "ctrl".

> Overall, this is more subtle than a simple g_strndup() solution.  But it
> doesn't quite reach the threshold for me asking you to redo it
> differently.
> 
> I can work in the two changes I proposed on commit, if you like them:
> dumb down the test for "<", and add the assertion.

Sounds good.