Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Jan 08, 2016 at 07:29:31PM +0530, P J P wrote:
> +-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+
> | Ah yes, how could I miss that. Maybe just add a min() around the
> | keyname_len computation?
> | 
> | - keyname_len = separator ? separator - keys : strlen(keys);
> | + keyname_len = MIN(sizeof(keyname_buf), separator ? separator - keys : strlen(keys))
> 
>   Actually, only use for 'keyname_len' is in the subsequent if statement, 

And the replacing of the minus in combined keys.

> which IIUC compares the keyname_buf for "<" key. Maybe it could say
> 
>   + if (!strncmp(keyname_buf, "<-", 2))
> 
> and remove the 'keyname_len' altogether.

This wouldn't catch '<' without '-'. (`sendkey <`)
Also, strncmp with a length of 1 (in the original) seems weird.

In the end my MIN() approach would be too forgiving when a key name is
longer than keyname_buf and its 15-byte substring exists (which I
doubt, though). Ie. {15chars}{and more} would be treated as {15chars}.
Worse with {15chars}{and more}-{anotherkey}.

keyname_len is not useless and perhaps it would be best to just do an
early error check there as I do below.
This makes sure no OOB write happens and changes the error output to
include the 'keys' part instead of keyname_buf. We can do this because
the two other `goto err_out` cases happen after using keyname_buf which
had been pstrcpy()d from the last keys, so 'keys' will contain the same
or more info, and works better for the new case since a string that is
too long might be cut off and then only part of it is displayed if the
input is say a combination of one too-long (thus invalid) key and a
second one.

Alternatively the if() can simply happen after pstrcpy() as a cut-off
error should be good enough anyway.

@@ -1749,6 +1749,9 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
     while (1) {
         separator = strchr(keys, '-');
         keyname_len = separator ? separator - keys : strlen(keys);
+        if (keyname_len >= sizeof(keyname_buf))
+            goto err_out;
+
         pstrcpy(keyname_buf, sizeof(keyname_buf), keys);

         /* Be compatible with old interface, convert user inputted "<" */
@@ -1800,7 +1803,7 @@ out:
     return;

 err_out:
-    monitor_printf(mon, "invalid parameter: %s\n", keyname_buf);
+    monitor_printf(mon, "invalid parameter: %s\n", keys);
     goto out;
 }