i386: fix handling of multiboot modules
cover i386: fix handling of multiboot modules
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse the initrd_filename once for multiboot modules
opts: remove redundant check for NULL parameter
hw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0
PATCH 1/3 i386: fix regression parsing multiboot initrd modules
The logic for parsing the multiboot initrd modules was messed up in
commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100
opts: don't silently truncate long option values
Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to get_opt_value() which was not robust at accepting a NULL value.
--- hw/i386/multiboot.c | 3 +-- util/qemu-option.c | 4 +++- 2 files changed, 4 insertions(+), 3 deletions(-)
hw/i386/multiboot.c +1 -2
@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg, cmdline_len +=strlen (kernel_cmdline) +1 ;if (initrd_filename) {const char *r =get_opt_value (initrd_filename,NULL ); - cmdline_len +=strlen (r) +1 ; - mbs.mb_mods_avail =1 ; + cmdline_len +=strlen (initrd_filename) +1 ;while (1 ) { mbs.mb_mods_avail++; r =get_opt_value (r,NULL );
util/qemu-option.c +3 -1
@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)size_t capacity =0 , length;const char *offset; - *value =NULL ; +if (value) { + *value =NULL ; + }while (1 ) { offset =strchr (p,',' );if (!offset) {
Signature · 1 line
2.17.0
Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
Quoted 37 lines · Daniel P. Berrangé
The logic for parsing the multiboot initrd modules was messed up in
commit
950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100opts: don't silently truncate long option values
Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to
get_opt_value()which was not robust at accepting a NULL value.Signed-off-by Daniel P. Berrangé <[email protected]>--- hw/i386/multiboot.c | 3 +-- util/qemu-option.c | 4 +++- 2 files changed, 4 insertions(+), 3 deletions(-)hw/i386/multiboot.c +1
-2@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg, cmdline_len +=strlen (kernel_cmdline) +1 ;if (initrd_filename) {const char *r =get_opt_value (initrd_filename,NULL ); - cmdline_len +=strlen (r) +1 ; - mbs.mb_mods_avail =1 ; + cmdline_len +=strlen (initrd_filename) +1 ;while (1 ) { mbs.mb_mods_avail++; r =get_opt_value (r,NULL );util/qemu-option.c +0
-0
util/qemu-option.c const char *get_opt_value(const char *p, char **value) · line 83
Quoted 10 lines
util/qemu-option.c +3
-1@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)size_t capacity =0 , length;const char *offset; - *value =NULL ; +if (value) { + *value =NULL ; + }while (1 ) { offset =strchr (p,',' );if (!offset) {
Don't we delete this check again in patch 3? If we're going to fix this by making multiboot.c not pass in NULL pointers, is there a reason not to simply do that?
thanks
-- PMM
Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
Quoted 41 lines · Peter Maydell
Quoted 40 lines · Daniel P. Berrangé
The logic for parsing the multiboot initrd modules was messed up in
commit
950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100opts: don't silently truncate long option values
Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to
get_opt_value()which was not robust at accepting a NULL value.Signed-off-by Daniel P. Berrangé <[email protected]>--- hw/i386/multiboot.c | 3 +-- util/qemu-option.c | 4 +++- 2 files changed, 4 insertions(+), 3 deletions(-)hw/i386/multiboot.c +1
-2@@ -292,8 +292,7 @@ int load_multiboot(FWCfgState *fw_cfg, cmdline_len +=strlen (kernel_cmdline) +1 ;if (initrd_filename) {const char *r =get_opt_value (initrd_filename,NULL ); - cmdline_len +=strlen (r) +1 ; - mbs.mb_mods_avail =1 ; + cmdline_len +=strlen (initrd_filename) +1 ;while (1 ) { mbs.mb_mods_avail++; r =get_opt_value (r,NULL );util/qemu-option.c +0
-0@@ -75,7 +75,9 @@ const char *get_opt_value(const char *p, char **value)size_t capacity =0 , length;const char *offset;
Quoted 11 lines
util/qemu-option.c const char *get_opt_value(const char *p, char **value) · line 83
Quoted 7 lines
util/qemu-option.c +3
-1- *value =NULL ; +if (value) { + *value =NULL ; + }while (1 ) { offset =strchr (p,',' );if (!offset) {Don't we delete this check again in patch 3? If we're going to fix this by making
multiboot.cnot pass in NULL pointers, is there a reason not to simply do that?
That is correct, however, I wanted to do a really simple fix that addressed the crasher regression, so that aspect of the fix wasn't obscured by the bigger refactoring I do in 2 & 3.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
In reply to Daniel P. BerrangéQuoted 6 lines · Daniel P. Berrangé
The logic for parsing the multiboot initrd modules was messed up in
commit
950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <[email protected]>
Date: Mon Apr 16 12:17:43 2018 +0100
Quoted 7 lines
opts: don't silently truncate long option values
Causing the length to be undercounter, and the number of modules over counted. It also passes NULL to
get_opt_value()which was not robust at accepting a NULL value.Signed-off-by Daniel P. Berrangé <[email protected]>
Queueing on x86-next. Thanks.
Signature · 1 line
Eduardo
PATCH 2/3 i386: only parse the initrd_filename once for multiboot modules
In reply to Daniel P. BerrangéThe multiboot code parses the initrd_filename twice, first to count how many entries there are, and second to process each entry. This changes the first loop to store the parse module names in a list, and the second loop can now use these names. This avoids having to pass NULL to the get_opt_value() method which means it can safely assume a non-NULL param.
--- hw/i386/multiboot.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)
hw/i386/multiboot.c +15 -17
@@ -161,6 +161,7 @@ int load_multiboot(FWCfgState *fw_cfg,uint8_t bootinfo[MBI_SIZE ];uint8_t *mb_bootinfo_data;uint32_t cmdline_len; +GList *mods =NULL ;/* Ok, let's see if it is a multiboot image. The header is 12x32bit long, so the latest entry may be 8192 - 48. */
@@ -291,15 +292,16 @@ int load_multiboot(FWCfgState *fw_cfg, cmdline_len =strlen (kernel_filename) +1 ; cmdline_len +=strlen (kernel_cmdline) +1 ;if (initrd_filename) { -const char *r =get_opt_value (initrd_filename,NULL ); +const char *r = initrd_filename; cmdline_len +=strlen (initrd_filename) +1 ; -while (1 ) { +while (*r) { +char *value; + r =get_opt_value (r, &value); mbs.mb_mods_avail++; - r =get_opt_value (r,NULL ); -if (!*r) { -break ; + mods =g_list_append (mods, value); +if (*r) { + r++; } - r++; } }
@@ -314,20 +316,16 @@ int load_multiboot(FWCfgState *fw_cfg, mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail *MB_MOD_SIZE ; mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len; -if (initrd_filename) { -const char *next_initrd; -char not_last; -char *one_file =NULL ; - +if (mods) { +GList *tmpl = mods; mbs.offset_mods = mbs.mb_buf_size; - do { +while (tmpl) {char *next_space;int mb_mod_length;uint32_t offs = mbs.mb_buf_size; +char *one_file = tmpl->data; - next_initrd = get_opt_value(initrd_filename, &one_file); - not_last = *next_initrd;/* if a space comes after the module filename, treat everything after that as parameters */ hwaddr c =mb_add_cmdline (&mbs, one_file);
@@ -352,10 +350,10 @@ int load_multiboot(FWCfgState *fw_cfg,mb_debug ("mod_start: %p\nmod_end: %p\n cmdline: " TARGET_FMT_plx, (char *)mbs.mb_buf + offs, (char *)mbs.mb_buf + offs + mb_mod_length, c); - initrd_filename = next_initrd+1 ;g_free (one_file); - one_file =NULL ; - }while (not_last); + tmpl = tmpl->next; + } +g_list_free (mods); }/* Commandline support */
Signature · 1 line
2.17.0
Re: [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules
Quoted 7 lines · Daniel P. Berrangé
The multiboot code parses the
initrd_filenametwice, first to count how many entries there are, and second to process each entry. This changes the first loop to store the parse module names in a list, and the second loop can now use these names. This avoids having to pass NULL to theget_opt_value()method which means it can safely assume a non-NULL param.Signed-off-by Daniel P. Berrangé <[email protected]>
[...]
hw/i386/multiboot.c int load_multiboot(FWCfgState *fw_cfg, · line 356
Quoted 11 lines
hw/i386/multiboot.c +3
-3@@ -352,10 +350,10 @@ int load_multiboot(FWCfgState *fw_cfg,mb_debug ("mod_start: %p\nmod_end: %p\n cmdline: " TARGET_FMT_plx, (char *)mbs.mb_buf + offs, (char *)mbs.mb_buf + offs + mb_mod_length, c); - initrd_filename = next_initrd+1 ;g_free (one_file); - one_file =NULL ; - }while (not_last); + tmpl = tmpl->next; + } +g_list_free (mods);
I thought this would cause double free of tmpl->data, but g_list_free() won't free the elements' data.
Queueing on x86-next.
Signature · 1 line
Eduardo
PATCH 3/3 opts: remove redundant check for NULL parameter
In reply to Daniel P. BerrangéNo callers of get_opt_value() pass in a NULL for the "value" parameter, so the check is redundant.
--- util/qemu-option.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
util/qemu-option.c +4 -8
@@ -75,9 +75,7 @@ const char *get_opt_value(const char *p, char **value)size_t capacity =0 , length;const char *offset; -if (value) { - *value =NULL ; - } + *value =NULL ;while (1 ) { offset =strchr (p,',' );if (!offset) {
@@ -88,11 +86,9 @@ const char *get_opt_value(const char *p, char **value)if (*offset !='\0' && *(offset +1 ) ==',' ) { length++; } -if (value) { - *value =g_renew (char, *value, capacity + length +1 ); -strncpy (*value + capacity, p, length); - (*value)[capacity + length] ='\0' ; - } + *value =g_renew (char, *value, capacity + length +1 ); +strncpy (*value + capacity, p, length); + (*value)[capacity + length] ='\0' ; capacity += length;if (*offset =='\0' || *(offset +1 ) !=',' ) {
Signature · 1 line
2.17.0
Re: [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter
Quoted 4 lines · Daniel P. Berrangé
No callers of
get_opt_value()pass in a NULL for the "value" parameter, so the check is redundant.Signed-off-by Daniel P. Berrangé <[email protected]>
Queueing on x86-next.
Signature · 1 line
Eduardo
ping...
Quoted 15 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterhw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 17 lines · Daniel P. Berrangé
ping...
Quoted 14 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterhw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0
Any reason this hasn't been merged yet?
This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.
Thanks,
Roman.
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé
ping...
Quoted 8 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameter
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
hw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0Any reason this hasn't been merged yet?
Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.
Quoted 3 lines
This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.
As it is a bug fix, I will try to review merge it for the next rc.
Signature · 1 line
Eduardo
Quoted 26 lines · Eduardo Habkost
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé
ping...
Quoted 8 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterQuoted 8 lines
Quoted 6 lines
Quoted 6 lines
hw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0Any reason this hasn't been merged yet?
Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.
Quoted 1 line
Quoted 5 lines
Quoted 2 lines
This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.
As it is a bug fix, I will try to review merge it for the next rc.
Looks like it ended up missing 3.0, so kvm-unit-tests still segfault with the released version of QEMU :(
Roman.
Quoted 27 lines · Roman Kagan
Quoted 26 lines · Eduardo Habkost
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé
ping...
Quoted 8 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterQuoted 8 lines
Quoted 6 lines
Quoted 6 lines
hw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0Any reason this hasn't been merged yet?
Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.
As it is a bug fix, I will try to review merge it for the next rc.
Looks like it ended up missing 3.0, so kvm-unit-tests still segfault with the released version of QEMU :(
Please check again, as I see it merged before 3.0:
commit 59b5552f020b739e273e969a0933c23d8f4e2284
Merge: ccf02d73d1 dfaa7d50b0
Author: Peter Maydell <[email protected]>
Date: Tue Jul 17 17:06:32 2018 +0100
Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging
Bug fixes.
# gpg: Signature made Tue 17 Jul 2018 16:06:07 BST
# gpg: using RSA key BFFBD25F78C7AE83
# gpg: Good signature from "Paolo Bonzini <[email protected]>"
# gpg: aka "Paolo Bonzini <[email protected]>"
# Primary key fingerprint: 46F5 9FBD 57D6 12E7 BFD4 E2F7 7E15 100C CD36 69B1
# Subkey fingerprint: F133 3857 4B66 2389 866C 7682 BFFB D25F 78C7 AE83
* remotes/bonzini/tags/for-upstream:
Document command line options with single dash
opts: remove redundant check for NULL parameter
i386: only parse the initrd_filename once for multiboot modules
i386: fix regression parsing multiboot initrd modules
virtio-scsi: fix hotplug ->reset() vs event race
qdev: add HotplugHandler->post_plug() callback
hw/char/serial: retry write if EAGAIN
PC Chipset: Improve serial divisor calculation
vhost-user-test: added proper TestServer *dest initialization in test_migrate()
hyperv: ensure VP index equal to QEMU cpu_index
hyperv: rename vcpu_id to vp_index
accel: Fix typo and grammar in comment
dump: add kernel_gs_base to QEMU CPU state
$ git describe 59b5552f020b739e273e969a0933c23d8f4e2284 v3.0.0-rc0-80-g59b5552f02
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Quoted 31 lines · Daniel P. Berrangé
Quoted 30 lines · Roman Kagan
Quoted 29 lines · Eduardo Habkost
Quoted 12 lines · Roman Kagan
Quoted 11 lines · Daniel P. Berrangé
ping...
Quoted 8 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterQuoted 8 lines
Quoted 6 lines
Quoted 6 lines
hw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0Any reason this hasn't been merged yet?
Lack of reviews, unfortunately. I guess we don't have many people familiar with the multiboot code.
Quoted 3 lines
This fixes a regression that, in particular, makes QEMU segfault when running kvm-unit-tests.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
As it is a bug fix, I will try to review merge it for the next rc.
Looks like it ended up missing 3.0, so kvm-unit-tests still segfault with the released version of QEMU :(
Please check again, as I see it merged before 3.0:
Oh, you're right, I just ran the tests against a wrong QEMU.
It's OK now indeed.
Sorry for the noise,
Roman.
Quoted 2 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
This flaw also makes QEMU segfault when running kvm-unit-tests, and the series fixes it.
So
Wondering if anyone else runs kvm-unit-tests with QEMU master, Roman.
Quoted 14 lines
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterhw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0
Quoted 7 lines · Roman Kagan
Quoted 2 lines · Daniel P. Berrangé
This short series fixes a flaw identified by coverity which broke the handling of multiboot modules.
This flaw also makes QEMU segfault when running kvm-unit-tests, and the series fixes it.
Quoted 6 lines
So
Tested-by Roman Kagan <[email protected]>Wondering if anyone else runs kvm-unit-tests with QEMU master, Roman.
It might be a good idea to make it a submodule, and use it for unit tests. I'll take a look.
Quoted 14 lines
Quoted 14 lines
Daniel P. Berrangé (3):
i386: fix regression parsing multiboot initrd modules
i386: only parse theinitrd_filenameonce for multiboot modules
opts: remove redundant check for NULL parameterhw/i386/multiboot.c | 35 ++++++++++++++++------------------- util/qemu-option.c | 8 +++----- 2 files changed, 19 insertions(+), 24 deletions(-)Signature · 1 line
2.17.0