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'?
> > - 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 :-)
> > 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".
> 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.
