Refactor und fix do_sendkey
PATCH 1/2 Refactor und fix do_sendkey
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.
--- monitor.c | 52 +++++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 23 deletions(-)
monitor.c +29 -23
@@ -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" },
Please avoid strncpy. Use pstrcpy intead.
Fabrice.
Quoted 5 lines · 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.
[...]
Quoted 1 line · Fabrice Bellard
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.
--- monitor.c | 52 +++++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 23 deletions(-)
monitor.c +29 -23
@@ -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" },
In message: <[email protected]>
Jan Kiszka <[email protected]> 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 <[email protected]>
: ---
: 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" },
:
:
:
Quoted 7 lines · Jan Kiszka
Quoted 1 line · Fabrice Bellard
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.
--- monitor.c | 51 ++++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 23 deletions(-)
monitor.c +28 -23
@@ -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" },