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

[Markus Armbruster]

Wolfgang Bumiller <address@hidden> writes:

> 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'?

Merely unclean, not actually broken in a practical sense.

>> > -        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 :-)

The assertion primarily documents we've considered strtoul() reading
beyond the bound.  It might also protects us from hasty, incorrect
changes in the future, but I guess that's secondary in this case.

Preexisting: we don't check errno.  Out of scope for this patch.

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

You're right.

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

Applied to my monitor-next with these tweaks:

diff --git a/hmp.c b/hmp.c
index 8be03df..9c571f5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1739,7 +1739,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
         keyname_len = separator ? separator - keys : strlen(keys);
 
         /* Be compatible with old interface, convert user inputted "<" */
-        if (!strncmp(keys, "<", 1) && keyname_len == 1) {
+        if (keys[0] == '<' && keyname_len == 1) {
             keys = "less";
             keyname_len = 4;
         }
@@ -1758,7 +1758,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
         if (strstart(keys, "0x", NULL)) {
             char *endp;
             int value = strtoul(keys, &endp, 0);
-            if (*endp != '\0' && *endp != '-') {
+            assert(endp <= keys + keyname_len);
+            if (endp != keys + keyname_len) {
                 goto err_out;
             }
             keylist->value->type = KEY_VALUE_KIND_NUMBER;