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