Remove artificial length limits when parsing options

Instead of re-using the get_opt_name() method from QemuOpts to split a string on ':', just use g_strsplit().

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 accel/accel.c         | 16 +++++++---------
 include/qemu/option.h |  1 -
 util/qemu-option.c    |  3 ++-
 3 files changed, 9 insertions(+), 11 deletions(-)

accel/accel.c +7 -9

index 93e2434c87..4981e7fff3 100644
@@ -70,8 +70,8 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
 
 void configure_accelerator(MachineState *ms)
 {
-    const char *accel, *p;
-    char buf[10];
+    const char *accel;
+    char **accel_list, **tmp;
     int ret;
     bool accel_initialised = false;
     bool init_failed = false;

@@ -83,13 +83,10 @@ void configure_accelerator(MachineState *ms)
         accel = "tcg";
     }
 
-    p = accel;
-    while (!accel_initialised && *p != '\0') {
-        if (*p == ':') {
-            p++;
-        }
-        p = get_opt_name(buf, sizeof(buf), p, ':');
-        acc = accel_find(buf);
+    accel_list = g_strsplit(accel, ":", 0);
+
+    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
+        acc = accel_find(*tmp);
         if (!acc) {
             continue;
         }

@@ -107,6 +104,7 @@ void configure_accelerator(MachineState *ms)
             accel_initialised = true;
         }
     }
+    g_strfreev(accel_list);
 
     if (!accel_initialised) {
         if (!init_failed) {

include/qemu/option.h +0 -1

index 306fdb5f7a..1cfe5cbc2d 100644
@@ -28,7 +28,6 @@
 
 #include "qemu/queue.h"
 
-const char *get_opt_name(char *buf, int buf_size, const char *p, char delim);
 const char *get_opt_value(char *buf, int buf_size, const char *p);
 
 void parse_option_size(const char *name, const char *value,

util/qemu-option.c +2 -1

index d0756fda58..baca40fb94 100644
@@ -49,7 +49,8 @@
  * The return value is the position of the delimiter/zero byte after the option
  * name in p.
  */
-const char *get_opt_name(char *buf, int buf_size, const char *p, char delim)
+static const char *get_opt_name(char *buf, int buf_size, const char *p,
+                                char delim)
 {
     char *q;
 

A user trying out SMBIOS "OEM strings" feature reported that the data they are exposing to the guest was truncated at 1023 bytes, which breaks the app consuming in the guest. After searching for the cause I eventually found that the QemuOpts parsing is using fixed length 1024 byte array for option values and 128 byte array for key names.

We can certainly debate whether it is sane to have such long command line argument values (it is not sane), but if the OS was capable of exec'ing QEMU with such an ARGV array, there is little good reason for imposing an artificial length restriction when parsing it. Even worse is that we silently truncate without reporting an error when hitting limits resulting in a semantically incorrect behaviour, possibly even leading to security flaws depending on the data that was truncated.

Thus this patch series removes the artificial length limits by killing the fixed length buffers.

Separately I intend to make it possible to read "OEM strings" data from a file, to avoid need to have long command line args.

Daniel P. Berrangé (3):
accel: use g_strsplit for parsing accelerator names
opts: don't silently truncate long parameter keys
opts: don't silently truncate long option values

 accel/accel.c          |  16 +++---
 hw/i386/multiboot.c    |  33 +++++++----
 include/qemu/option.h  |   3 +-
 tests/test-qemu-opts.c |  18 ------
 util/qemu-option.c     | 150 ++++++++++++++++++++++++++-----------------------
 5 files changed, 108 insertions(+), 112 deletions(-)

A user trying out SMBIOS "OEM strings" feature reported that the data they are exposing to the guest was truncated at 1023 bytes, which breaks the app consuming in the guest. After searching for the cause I eventually found that the QemuOpts parsing is using fixed length 1024 byte array for option values and 128 byte array for key names.

We can certainly debate whether it is sane to have such long command line argument values (it is not sane), but if the OS was capable of exec'ing QEMU with such an ARGV array, there is little good reason for imposing an artificial length restriction when parsing it. Even worse is that we silently truncate without reporting an error when hitting limits resulting in a semantically incorrect behaviour, possibly even leading to security flaws depending on the data that was truncated.

Thus this patch series removes the artificial length limits by killing the fixed length buffers.

Separately I intend to make it possible to read "OEM strings" data from a file, to avoid need to have long command line args.

Quoted 17 lines · Daniel P. Berrangé

A user trying out SMBIOS "OEM strings" feature reported that the data they are exposing to the guest was truncated at 1023 bytes, which breaks the app consuming in the guest. After searching for the cause I eventually found that the QemuOpts parsing is using fixed length 1024 byte array for option values and 128 byte array for key names.

We can certainly debate whether it is sane to have such long command line argument values (it is not sane), but if the OS was capable of exec'ing QEMU with such an ARGV array, there is little good reason for imposing an artificial length restriction when parsing it. Even worse is that we silently truncate without reporting an error when hitting limits resulting in a semantically incorrect behaviour, possibly even leading to security flaws depending on the data that was truncated.

Thus this patch series removes the artificial length limits by killing the fixed length buffers.

Quoted 2 lines

Separately I intend to make it possible to read "OEM strings" data from a file, to avoid need to have long command line args.

Too bad I haven't been able to complete my quest to kill QemuOpts.

As far as I know, keyval.c's only arbitrary limit is the length of a key fragment (the things separated by '.').

Quoted 19 lines · Markus Armbruster

Quoted 17 lines · Daniel P. Berrangé

A user trying out SMBIOS "OEM strings" feature reported that the data they are exposing to the guest was truncated at 1023 bytes, which breaks the app consuming in the guest. After searching for the cause I eventually found that the QemuOpts parsing is using fixed length 1024 byte array for option values and 128 byte array for key names.

We can certainly debate whether it is sane to have such long command line argument values (it is not sane), but if the OS was capable of exec'ing QEMU with such an ARGV array, there is little good reason for imposing an artificial length restriction when parsing it. Even worse is that we silently truncate without reporting an error when hitting limits resulting in a semantically incorrect behaviour, possibly even leading to security flaws depending on the data that was truncated.

Thus this patch series removes the artificial length limits by killing the fixed length buffers.

Quoted 7 lines

Quoted 2 lines

Separately I intend to make it possible to read "OEM strings" data from a file, to avoid need to have long command line args.

Too bad I haven't been able to complete my quest to kill QemuOpts.

As far as I know, keyval.c's only arbitrary limit is the length of a key fragment (the things separated by '.').

Looks like that's the same scenario I tried to address in patch 2. The 'key' part in QemuOpts has the same 128 byte limit as in the keyval.c code. I fear that could be hit with -blockdev when setting params on very deeply nested block backends. On the plus side keyval.c actually reports an error when it hits its 128 byte limit, instead of silently carrying on as if all was well like QemuOpts did :-)

Quoted 28 lines · Daniel P. Berrangé

Quoted 19 lines · Markus Armbruster

Quoted 17 lines · Daniel P. Berrangé

A user trying out SMBIOS "OEM strings" feature reported that the data they are exposing to the guest was truncated at 1023 bytes, which breaks the app consuming in the guest. After searching for the cause I eventually found that the QemuOpts parsing is using fixed length 1024 byte array for option values and 128 byte array for key names.

We can certainly debate whether it is sane to have such long command line argument values (it is not sane), but if the OS was capable of exec'ing QEMU with such an ARGV array, there is little good reason for imposing an artificial length restriction when parsing it. Even worse is that we silently truncate without reporting an error when hitting limits resulting in a semantically incorrect behaviour, possibly even leading to security flaws depending on the data that was truncated.

Thus this patch series removes the artificial length limits by killing the fixed length buffers.

Quoted 7 lines

Quoted 2 lines

Separately I intend to make it possible to read "OEM strings" data from a file, to avoid need to have long command line args.

Too bad I haven't been able to complete my quest to kill QemuOpts.

As far as I know, keyval.c's only arbitrary limit is the length of a key fragment (the things separated by '.').

Quoted 6 lines

Looks like that's the same scenario I tried to address in patch 2. The 'key' part in QemuOpts has the same 128 byte limit as in the keyval.c code. I fear that could be hit with -blockdev when setting params on very deeply nested block backends. On the plus side keyval.c actually reports an error when it hits its 128 byte limit, instead of silently carrying on as if all was well like QemuOpts did :-)

In keyval.c, the key (things like "a.b.c") can be arbitrarily long (well, until g_malloc() throws in the towel), but each key fragment ("a", "b" and "c") is limited to 128 bytes.

If key length was limited there, I would've asked you to fix it there, too.