diff mbox

hmp: avoid redundant null termination of buffer

Message ID 87oacjklvb.fsf@blackfin.pond.sub.org (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Armbruster Jan. 18, 2016, 2:23 p.m. UTC
Wolfgang Bumiller <w.bumiller@proxmox.com> writes:

> On Mon, Jan 18, 2016 at 02:02:07PM +0100, Markus Armbruster wrote:
>> Wolfgang Bumiller <w.bumiller@proxmox.com> writes:
>> 
>> > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote:
>> >> Wolfgang Bumiller <w.bumiller@proxmox.com> 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:

Comments

Michael Tokarev Jan. 26, 2016, 9:36 a.m. UTC | #1
18.01.2016 17:23, Markus Armbruster wrote:
[...]
> 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;

Marcus, where's your monitor-next branch?  Repository at
git://repo.or.cz/qemu/armbru.git , monitor-next branch does
not contain this change, last commit to hmp.c dated Sep-8.

Thanks,

/mjt
Michael Tokarev Jan. 28, 2016, 10:52 a.m. UTC | #2
Ping?

26.01.2016 12:36, Michael Tokarev wrote:
> 18.01.2016 17:23, Markus Armbruster wrote:
> [...]
>> 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;
> 
> Marcus, where's your monitor-next branch?  Repository at
> git://repo.or.cz/qemu/armbru.git , monitor-next branch does
> not contain this change, last commit to hmp.c dated Sep-8.
> 
> Thanks,
> 
> /mjt
>
Markus Armbruster Jan. 28, 2016, 2:45 p.m. UTC | #3
Michael Tokarev <mjt@tls.msk.ru> writes:

> Ping?

Sorry for the delay, I've been focusing on the QAPI queue to the
exclusion of pretty much everything else.

> 26.01.2016 12:36, Michael Tokarev wrote:
>> 18.01.2016 17:23, Markus Armbruster wrote:
>> [...]
>>> 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;
>> 
>> Marcus, where's your monitor-next branch?  Repository at
>> git://repo.or.cz/qemu/armbru.git , monitor-next branch does
>> not contain this change, last commit to hmp.c dated Sep-8.

I forgot to push.  Should be there now, but needs a rebase to current
master.
diff mbox

Patch

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;