[Qemu-devel] [PATCH 1/2] Refactor und fix do_sendkey

[Qemu-devel] [PATCH 1/2] Refactor und fix do_sendkey

[Jan Kiszka]

Looking at the sendkey implementation, planning to enhance it with a
hold time argument, I found some potential out-of-bound access and not
very readable code. Here is a fix for the former and a (subjective)
improvement of the latter.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 monitor.c |   52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

Index: b/monitor.c
===================================================================
--- a/monitor.c
+++ b/monitor.c
@@ -925,33 +925,39 @@ static int get_keycode(const char *key)
     return -1;
 }
 
-static void do_send_key(const char *string)
+static void do_sendkey(const char *string)
 {
-    char keybuf[16], *q;
     uint8_t keycodes[16];
-    const char *p;
-    int nb_keycodes, keycode, i;
-
-    nb_keycodes = 0;
-    p = string;
-    while (*p != '\0') {
-        q = keybuf;
-        while (*p != '\0' && *p != '-') {
-            if ((q - keybuf) < sizeof(keybuf) - 1) {
-                *q++ = *p;
+    int nb_keycodes = 0;
+    char keyname_buf[16];
+    char *separator;
+    int keyname_len, keycode, i;
+
+    while (1) {
+        separator = strchr(string, '-');
+        keyname_len = separator ? separator-string : strlen(string);
+        if (keyname_len > 0) {
+            strncpy(keyname_buf, string, sizeof(keyname_buf) - 1);
+            if (keyname_len > sizeof(keyname_buf) - 1) {
+                keyname_buf[sizeof(keyname_buf) - 1] = 0;
+                term_printf("invalid key: '%s...'\n", keyname_buf);
+                return;
             }
-            p++;
-        }
-        *q = '\0';
-        keycode = get_keycode(keybuf);
-        if (keycode < 0) {
-            term_printf("unknown key: '%s'\n", keybuf);
-            return;
+            if (nb_keycodes == sizeof(keycodes)) {
+                term_printf("too many keys\n");
+                return;
+            }
+            keyname_buf[keyname_len] = 0;
+            keycode = get_keycode(keyname_buf);
+            if (keycode < 0) {
+                term_printf("unknown key: '%s'\n", keyname_buf);
+                return;
+            }
+            keycodes[nb_keycodes++] = keycode;
         }
-        keycodes[nb_keycodes++] = keycode;
-        if (*p == '\0')
+        if (!separator)
             break;
-        p++;
+        string = separator + 1;
     }
     /* key down events */
     for(i = 0; i < nb_keycodes; i++) {
@@ -1353,7 +1359,7 @@ static term_cmd_t term_cmds[] = {
     { "i", "/ii.", do_ioport_read,
       "/fmt addr", "I/O port read" },
 
-    { "sendkey", "s", do_send_key,
+    { "sendkey", "s", do_sendkey,
       "keys", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1')" },
     { "system_reset", "", do_system_reset,
       "", "reset the system" },

Re: [Qemu-devel] [PATCH 1/2] Refactor und fix do_sendkey

[Fabrice Bellard]

Please avoid strncpy. Use pstrcpy intead.

Fabrice.

Jan Kiszka wrote:
> Looking at the sendkey implementation, planning to enhance it with a
> hold time argument, I found some potential out-of-bound access and not
> very readable code. Here is a fix for the former and a (subjective)
> improvement of the latter.
> [...]

[Qemu-devel] Re: [PATCH 1/2] Refactor und fix do_sendkey

[Jan Kiszka]

Fabrice Bellard wrote:
> Please avoid strncpy. Use pstrcpy intead.

Ah, non-POSIX host platforms. Wouldn't it be easier to provide POSIX
wrappers for those few OSes?

However, fixed-up version follows:

-----------

Looking at the sendkey implementation, planning to enhance it with a
hold time argument, I found some potential out-of-bound access and not
very readable code. Here is a fix for the former and a (subjective)
improvement of the latter.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 monitor.c |   52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

Index: b/monitor.c
===================================================================
--- a/monitor.c
+++ b/monitor.c
@@ -925,33 +925,39 @@ static int get_keycode(const char *key)
     return -1;
 }
 
-static void do_send_key(const char *string)
+static void do_sendkey(const char *string)
 {
-    char keybuf[16], *q;
     uint8_t keycodes[16];
-    const char *p;
-    int nb_keycodes, keycode, i;
-
-    nb_keycodes = 0;
-    p = string;
-    while (*p != '\0') {
-        q = keybuf;
-        while (*p != '\0' && *p != '-') {
-            if ((q - keybuf) < sizeof(keybuf) - 1) {
-                *q++ = *p;
+    int nb_keycodes = 0;
+    char keyname_buf[16];
+    char *separator;
+    int keyname_len, keycode, i;
+
+    while (1) {
+        separator = strchr(string, '-');
+        keyname_len = separator ? separator-string : strlen(string);
+        if (keyname_len > 0) {
+            pstrcpy(keyname_buf, string, sizeof(keyname_buf) - 1);
+            if (keyname_len > sizeof(keyname_buf) - 1) {
+                keyname_buf[sizeof(keyname_buf) - 1] = 0;
+                term_printf("invalid key: '%s...'\n", keyname_buf);
+                return;
             }
-            p++;
-        }
-        *q = '\0';
-        keycode = get_keycode(keybuf);
-        if (keycode < 0) {
-            term_printf("unknown key: '%s'\n", keybuf);
-            return;
+            if (nb_keycodes == sizeof(keycodes)) {
+                term_printf("too many keys\n");
+                return;
+            }
+            keyname_buf[keyname_len] = 0;
+            keycode = get_keycode(keyname_buf);
+            if (keycode < 0) {
+                term_printf("unknown key: '%s'\n", keyname_buf);
+                return;
+            }
+            keycodes[nb_keycodes++] = keycode;
         }
-        keycodes[nb_keycodes++] = keycode;
-        if (*p == '\0')
+        if (!separator)
             break;
-        p++;
+        string = separator + 1;
     }
     /* key down events */
     for(i = 0; i < nb_keycodes; i++) {
@@ -1353,7 +1359,7 @@ static term_cmd_t term_cmds[] = {
     { "i", "/ii.", do_ioport_read,
       "/fmt addr", "I/O port read" },
 
-    { "sendkey", "s", do_send_key,
+    { "sendkey", "s", do_sendkey,
       "keys", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1')" },
     { "system_reset", "", do_system_reset,
       "", "reset the system" },

Re: [Qemu-devel] Re: [PATCH 1/2] Refactor und fix do_sendkey

[M. Warner Losh]

In message: <48359204.80200@web.de>
            Jan Kiszka <jan.kiszka@web.de> writes:
: Fabrice Bellard wrote:
: > Please avoid strncpy. Use pstrcpy intead.
: 
: Ah, non-POSIX host platforms. Wouldn't it be easier to provide POSIX
: wrappers for those few OSes?

strncpy is plain, vanilla C.  However, its use is dangerous and very
often used wrong.

Warner

: However, fixed-up version follows:
: 
: -----------
: 
: Looking at the sendkey implementation, planning to enhance it with a
: hold time argument, I found some potential out-of-bound access and not
: very readable code. Here is a fix for the former and a (subjective)
: improvement of the latter.
: 
: Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
: ---
:  monitor.c |   52 +++++++++++++++++++++++++++++-----------------------
:  1 file changed, 29 insertions(+), 23 deletions(-)
: 
: Index: b/monitor.c
: ===================================================================
: --- a/monitor.c
: +++ b/monitor.c
: @@ -925,33 +925,39 @@ static int get_keycode(const char *key)
:      return -1;
:  }
:  
: -static void do_send_key(const char *string)
: +static void do_sendkey(const char *string)
:  {
: -    char keybuf[16], *q;
:      uint8_t keycodes[16];
: -    const char *p;
: -    int nb_keycodes, keycode, i;
: -
: -    nb_keycodes = 0;
: -    p = string;
: -    while (*p != '\0') {
: -        q = keybuf;
: -        while (*p != '\0' && *p != '-') {
: -            if ((q - keybuf) < sizeof(keybuf) - 1) {
: -                *q++ = *p;
: +    int nb_keycodes = 0;
: +    char keyname_buf[16];
: +    char *separator;
: +    int keyname_len, keycode, i;
: +
: +    while (1) {
: +        separator = strchr(string, '-');
: +        keyname_len = separator ? separator-string : strlen(string);
: +        if (keyname_len > 0) {
: +            pstrcpy(keyname_buf, string, sizeof(keyname_buf) - 1);
: +            if (keyname_len > sizeof(keyname_buf) - 1) {
: +                keyname_buf[sizeof(keyname_buf) - 1] = 0;
: +                term_printf("invalid key: '%s...'\n", keyname_buf);
: +                return;
:              }
: -            p++;
: -        }
: -        *q = '\0';
: -        keycode = get_keycode(keybuf);
: -        if (keycode < 0) {
: -            term_printf("unknown key: '%s'\n", keybuf);
: -            return;
: +            if (nb_keycodes == sizeof(keycodes)) {
: +                term_printf("too many keys\n");
: +                return;
: +            }
: +            keyname_buf[keyname_len] = 0;
: +            keycode = get_keycode(keyname_buf);
: +            if (keycode < 0) {
: +                term_printf("unknown key: '%s'\n", keyname_buf);
: +                return;
: +            }
: +            keycodes[nb_keycodes++] = keycode;
:          }
: -        keycodes[nb_keycodes++] = keycode;
: -        if (*p == '\0')
: +        if (!separator)
:              break;
: -        p++;
: +        string = separator + 1;
:      }
:      /* key down events */
:      for(i = 0; i < nb_keycodes; i++) {
: @@ -1353,7 +1359,7 @@ static term_cmd_t term_cmds[] = {
:      { "i", "/ii.", do_ioport_read,
:        "/fmt addr", "I/O port read" },
:  
: -    { "sendkey", "s", do_send_key,
: +    { "sendkey", "s", do_sendkey,
:        "keys", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1')" },
:      { "system_reset", "", do_system_reset,
:        "", "reset the system" },
: 
: 
: 

[Qemu-devel] Re: [PATCH 1/2] Refactor und fix do_sendkey

[Jan Kiszka]

Jan Kiszka wrote:
> Fabrice Bellard wrote:
>> Please avoid strncpy. Use pstrcpy intead.
> 
> Ah, non-POSIX host platforms. Wouldn't it be easier to provide POSIX
> wrappers for those few OSes?
> 
> However, fixed-up version follows:

That version was crap, pstrcpy is much more different. This should be
better now:

------------

Looking at the sendkey implementation, planning to enhance it with a
hold time argument, I found some potential out-of-bound access and not
very readable code. Here is a fix for the former and a (subjective)
improvement of the latter.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 monitor.c |   51 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

Index: b/monitor.c
===================================================================
--- a/monitor.c
+++ b/monitor.c
@@ -925,33 +925,38 @@ static int get_keycode(const char *key)
     return -1;
 }
 
-static void do_send_key(const char *string)
+static void do_sendkey(const char *string)
 {
-    char keybuf[16], *q;
     uint8_t keycodes[16];
-    const char *p;
-    int nb_keycodes, keycode, i;
-
-    nb_keycodes = 0;
-    p = string;
-    while (*p != '\0') {
-        q = keybuf;
-        while (*p != '\0' && *p != '-') {
-            if ((q - keybuf) < sizeof(keybuf) - 1) {
-                *q++ = *p;
+    int nb_keycodes = 0;
+    char keyname_buf[16];
+    char *separator;
+    int keyname_len, keycode, i;
+
+    while (1) {
+        separator = strchr(string, '-');
+        keyname_len = separator ? separator-string : strlen(string);
+        if (keyname_len > 0) {
+            pstrcpy(keyname_buf, sizeof(keyname_buf), string);
+            if (keyname_len > sizeof(keyname_buf) - 1) {
+                term_printf("invalid key: '%s...'\n", keyname_buf);
+                return;
             }
-            p++;
-        }
-        *q = '\0';
-        keycode = get_keycode(keybuf);
-        if (keycode < 0) {
-            term_printf("unknown key: '%s'\n", keybuf);
-            return;
+            if (nb_keycodes == sizeof(keycodes)) {
+                term_printf("too many keys\n");
+                return;
+            }
+            keyname_buf[keyname_len] = 0;
+            keycode = get_keycode(keyname_buf);
+            if (keycode < 0) {
+                term_printf("unknown key: '%s'\n", keyname_buf);
+                return;
+            }
+            keycodes[nb_keycodes++] = keycode;
         }
-        keycodes[nb_keycodes++] = keycode;
-        if (*p == '\0')
+        if (!separator)
             break;
-        p++;
+        string = separator + 1;
     }
     /* key down events */
     for(i = 0; i < nb_keycodes; i++) {
@@ -1353,7 +1358,7 @@ static term_cmd_t term_cmds[] = {
     { "i", "/ii.", do_ioport_read,
       "/fmt addr", "I/O port read" },
 
-    { "sendkey", "s", do_send_key,
+    { "sendkey", "s", do_sendkey,
       "keys", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1')" },
     { "system_reset", "", do_system_reset,
       "", "reset the system" },