* [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
@ 2018-05-14 17:19 Daniel P. Berrangé
2018-05-14 17:19 ` [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules Daniel P. Berrangé
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-05-14 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Eduardo Habkost, Richard Henderson,
Paolo Bonzini, Marcel Apfelbaum, Markus Armbruster,
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 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(-)
--
2.17.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
2018-05-14 17:19 [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
@ 2018-05-14 17:19 ` Daniel P. Berrangé
2018-05-18 17:54 ` Peter Maydell
2018-07-16 21:23 ` Eduardo Habkost
2018-05-14 17:19 ` [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules Daniel P. Berrangé
` (3 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-05-14 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Eduardo Habkost, Richard Henderson,
Paolo Bonzini, Marcel Apfelbaum, Markus Armbruster,
Daniel P. Berrangé
The logic for parsing the multiboot initrd modules was messed up in
commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
Author: Daniel P. Berrangé <berrange@redhat.com>
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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
hw/i386/multiboot.c | 3 +--
util/qemu-option.c | 4 +++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 7a2953e26f..8e26545814 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -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);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 58d1c23893..8a68bc2314 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -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) {
--
2.17.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules
2018-05-14 17:19 [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
2018-05-14 17:19 ` [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules Daniel P. Berrangé
@ 2018-05-14 17:19 ` Daniel P. Berrangé
2018-07-16 21:28 ` Eduardo Habkost
2018-05-14 17:19 ` [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter Daniel P. Berrangé
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-05-14 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Eduardo Habkost, Richard Henderson,
Paolo Bonzini, Marcel Apfelbaum, Markus Armbruster,
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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
hw/i386/multiboot.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 8e26545814..d519e206c5 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -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 */
--
2.17.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter
2018-05-14 17:19 [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
2018-05-14 17:19 ` [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules Daniel P. Berrangé
2018-05-14 17:19 ` [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules Daniel P. Berrangé
@ 2018-05-14 17:19 ` Daniel P. Berrangé
2018-07-16 21:28 ` Eduardo Habkost
2018-06-07 9:47 ` [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
2018-06-20 14:57 ` Roman Kagan
4 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-05-14 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Eduardo Habkost, Richard Henderson,
Paolo Bonzini, Marcel Apfelbaum, Markus Armbruster,
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é <berrange@redhat.com>
---
util/qemu-option.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 8a68bc2314..a1ff682aac 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -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) != ',') {
--
2.17.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
2018-05-14 17:19 ` [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules Daniel P. Berrangé
@ 2018-05-18 17:54 ` Peter Maydell
2018-05-21 8:56 ` Daniel P. Berrangé
2018-07-16 21:23 ` Eduardo Habkost
1 sibling, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2018-05-18 17:54 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: QEMU Developers, Eduardo Habkost, Michael S. Tsirkin,
Markus Armbruster, Paolo Bonzini, Richard Henderson
On 14 May 2018 at 18:19, Daniel P. Berrangé <berrange@redhat.com> wrote:
> The logic for parsing the multiboot initrd modules was messed up in
>
> commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
> Author: Daniel P. Berrangé <berrange@redhat.com>
> 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.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> hw/i386/multiboot.c | 3 +--
> util/qemu-option.c | 4 +++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 7a2953e26f..8e26545814 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -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);
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 58d1c23893..8a68bc2314 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
2018-05-18 17:54 ` Peter Maydell
@ 2018-05-21 8:56 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-05-21 8:56 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Eduardo Habkost, Michael S. Tsirkin,
Markus Armbruster, Paolo Bonzini, Richard Henderson
On Fri, May 18, 2018 at 06:54:48PM +0100, Peter Maydell wrote:
> On 14 May 2018 at 18:19, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > The logic for parsing the multiboot initrd modules was messed up in
> >
> > commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
> > Author: Daniel P. Berrangé <berrange@redhat.com>
> > 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.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > hw/i386/multiboot.c | 3 +--
> > util/qemu-option.c | 4 +++-
> > 2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> > index 7a2953e26f..8e26545814 100644
> > --- a/hw/i386/multiboot.c
> > +++ b/hw/i386/multiboot.c
> > @@ -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);
> > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > index 58d1c23893..8a68bc2314 100644
> > --- a/util/qemu-option.c
> > +++ b/util/qemu-option.c
> > @@ -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?
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
--
|: 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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-05-14 17:19 [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
` (2 preceding siblings ...)
2018-05-14 17:19 ` [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter Daniel P. Berrangé
@ 2018-06-07 9:47 ` Daniel P. Berrangé
2018-07-10 17:11 ` Roman Kagan
2018-06-20 14:57 ` Roman Kagan
4 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-06-07 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Eduardo Habkost, Richard Henderson,
Paolo Bonzini, Marcel Apfelbaum, Markus Armbruster
ping...
On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> 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(-)
>
> --
> 2.17.0
>
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-05-14 17:19 [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
` (3 preceding siblings ...)
2018-06-07 9:47 ` [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
@ 2018-06-20 14:57 ` Roman Kagan
2018-06-20 15:34 ` Michael S. Tsirkin
4 siblings, 1 reply; 17+ messages in thread
From: Roman Kagan @ 2018-06-20 14:57 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
Markus Armbruster, Paolo Bonzini, Richard Henderson
On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> 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
Tested-by: Roman Kagan <rkagan@virtuozzo.com>
Wondering if anyone else runs kvm-unit-tests with QEMU master,
Roman.
>
> 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(-)
>
> --
> 2.17.0
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-06-20 14:57 ` Roman Kagan
@ 2018-06-20 15:34 ` Michael S. Tsirkin
0 siblings, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2018-06-20 15:34 UTC (permalink / raw)
To: Roman Kagan, Daniel P. Berrangé,
qemu-devel, Eduardo Habkost, Markus Armbruster, Paolo Bonzini,
Richard Henderson
On Wed, Jun 20, 2018 at 05:57:56PM +0300, Roman Kagan wrote:
> On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > 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
>
> Tested-by: Roman Kagan <rkagan@virtuozzo.com>
>
> 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.
> >
> > 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(-)
> >
> > --
> > 2.17.0
> >
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-06-07 9:47 ` [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
@ 2018-07-10 17:11 ` Roman Kagan
2018-07-10 17:23 ` Eduardo Habkost
0 siblings, 1 reply; 17+ messages in thread
From: Roman Kagan @ 2018-07-10 17:11 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Eduardo Habkost, Michael S. Tsirkin,
Markus Armbruster, Paolo Bonzini, Richard Henderson
On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> ping...
>
> On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > 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(-)
> >
> > --
> > 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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-07-10 17:11 ` Roman Kagan
@ 2018-07-10 17:23 ` Eduardo Habkost
2018-08-16 14:34 ` Roman Kagan
0 siblings, 1 reply; 17+ messages in thread
From: Eduardo Habkost @ 2018-07-10 17:23 UTC (permalink / raw)
To: Roman Kagan, Daniel P. Berrangé,
qemu-devel, Michael S. Tsirkin, Markus Armbruster, Paolo Bonzini,
Richard Henderson
On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > ping...
> >
> > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > 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(-)
> > >
> > > --
> > > 2.17.0
>
> Any reason this hasn't been merged yet?
Lack of reviews, unfortunately. I guess we don't have many
people familiar with the multiboot code.
>
> 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.
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules
2018-05-14 17:19 ` [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules Daniel P. Berrangé
2018-05-18 17:54 ` Peter Maydell
@ 2018-07-16 21:23 ` Eduardo Habkost
1 sibling, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2018-07-16 21:23 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Michael S. Tsirkin, Markus Armbruster, Paolo Bonzini,
Richard Henderson
On Mon, May 14, 2018 at 06:19:11PM +0100, Daniel P. Berrangé wrote:
> The logic for parsing the multiboot initrd modules was messed up in
>
> commit 950c4e6c94b15cd0d8b63891dddd7a8dbf458e6a
> Author: Daniel P. Berrangé <berrange@redhat.com>
> 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.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Queueing on x86-next. Thanks.
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules
2018-05-14 17:19 ` [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules Daniel P. Berrangé
@ 2018-07-16 21:28 ` Eduardo Habkost
0 siblings, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2018-07-16 21:28 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Michael S. Tsirkin, Markus Armbruster, Paolo Bonzini,
Richard Henderson
On Mon, May 14, 2018 at 06:19:12PM +0100, Daniel P. Berrangé wrote:
> 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.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[...]
> @@ -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.
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Queueing on x86-next.
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter
2018-05-14 17:19 ` [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter Daniel P. Berrangé
@ 2018-07-16 21:28 ` Eduardo Habkost
0 siblings, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2018-07-16 21:28 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Michael S. Tsirkin, Markus Armbruster, Paolo Bonzini,
Richard Henderson
On Mon, May 14, 2018 at 06:19:13PM +0100, Daniel P. Berrangé wrote:
> 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é <berrange@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Queueing on x86-next.
--
Eduardo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-07-10 17:23 ` Eduardo Habkost
@ 2018-08-16 14:34 ` Roman Kagan
2018-08-16 14:38 ` Daniel P. Berrangé
0 siblings, 1 reply; 17+ messages in thread
From: Roman Kagan @ 2018-08-16 14:34 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Daniel P. Berrangé,
qemu-devel, Michael S. Tsirkin, Markus Armbruster, Paolo Bonzini,
Richard Henderson
On Tue, Jul 10, 2018 at 02:23:09PM -0300, Eduardo Habkost wrote:
> On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> > On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > > ping...
> > >
> > > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > > 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(-)
> > > >
> > > > --
> > > > 2.17.0
> >
> > Any reason this hasn't been merged yet?
>
> Lack of reviews, unfortunately. I guess we don't have many
> people familiar with the multiboot code.
>
> >
> > 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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-08-16 14:34 ` Roman Kagan
@ 2018-08-16 14:38 ` Daniel P. Berrangé
2018-08-16 14:41 ` Roman Kagan
0 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2018-08-16 14:38 UTC (permalink / raw)
To: Roman Kagan, Eduardo Habkost, qemu-devel, Michael S. Tsirkin,
Markus Armbruster, Paolo Bonzini, Richard Henderson
On Thu, Aug 16, 2018 at 05:34:43PM +0300, Roman Kagan wrote:
> On Tue, Jul 10, 2018 at 02:23:09PM -0300, Eduardo Habkost wrote:
> > On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> > > On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > > > ping...
> > > >
> > > > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > > > 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(-)
> > > > >
> > > > > --
> > > > > 2.17.0
> > >
> > > Any reason this hasn't been merged yet?
> >
> > Lack of reviews, unfortunately. I guess we don't have many
> > people familiar with the multiboot code.
> >
> > >
> > > 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 <peter.maydell@linaro.org>
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 <bonzini@gnu.org>"
# gpg: aka "Paolo Bonzini <pbonzini@redhat.com>"
# 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
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
$ git describe 59b5552f020b739e273e969a0933c23d8f4e2284
v3.0.0-rc0-80-g59b5552f02
Regards,
Daniel
--
|: 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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules
2018-08-16 14:38 ` Daniel P. Berrangé
@ 2018-08-16 14:41 ` Roman Kagan
0 siblings, 0 replies; 17+ messages in thread
From: Roman Kagan @ 2018-08-16 14:41 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Eduardo Habkost, qemu-devel, Michael S. Tsirkin,
Markus Armbruster, Paolo Bonzini, Richard Henderson
On Thu, Aug 16, 2018 at 03:38:14PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 16, 2018 at 05:34:43PM +0300, Roman Kagan wrote:
> > On Tue, Jul 10, 2018 at 02:23:09PM -0300, Eduardo Habkost wrote:
> > > On Tue, Jul 10, 2018 at 08:11:58PM +0300, Roman Kagan wrote:
> > > > On Thu, Jun 07, 2018 at 10:47:47AM +0100, Daniel P. Berrangé wrote:
> > > > > ping...
> > > > >
> > > > > On Mon, May 14, 2018 at 06:19:10PM +0100, Daniel P. Berrangé wrote:
> > > > > > 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(-)
> > > > > >
> > > > > > --
> > > > > > 2.17.0
> > > >
> > > > Any reason this hasn't been merged yet?
> > >
> > > Lack of reviews, unfortunately. I guess we don't have many
> > > people familiar with the multiboot code.
> > >
> > > >
> > > > 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:
Oh, you're right, I just ran the tests against a wrong QEMU.
It's OK now indeed.
Sorry for the noise,
Roman.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-08-16 14:41 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-14 17:19 [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
2018-05-14 17:19 ` [Qemu-devel] [PATCH 1/3] i386: fix regression parsing multiboot initrd modules Daniel P. Berrangé
2018-05-18 17:54 ` Peter Maydell
2018-05-21 8:56 ` Daniel P. Berrangé
2018-07-16 21:23 ` Eduardo Habkost
2018-05-14 17:19 ` [Qemu-devel] [PATCH 2/3] i386: only parse the initrd_filename once for multiboot modules Daniel P. Berrangé
2018-07-16 21:28 ` Eduardo Habkost
2018-05-14 17:19 ` [Qemu-devel] [PATCH 3/3] opts: remove redundant check for NULL parameter Daniel P. Berrangé
2018-07-16 21:28 ` Eduardo Habkost
2018-06-07 9:47 ` [Qemu-devel] [PATCH 0/3] i386: fix handling of multiboot modules Daniel P. Berrangé
2018-07-10 17:11 ` Roman Kagan
2018-07-10 17:23 ` Eduardo Habkost
2018-08-16 14:34 ` Roman Kagan
2018-08-16 14:38 ` Daniel P. Berrangé
2018-08-16 14:41 ` Roman Kagan
2018-06-20 14:57 ` Roman Kagan
2018-06-20 15:34 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.