Markus Armbruster (2): pc_sysfw: Check for qemu_find_file() failure pc_sysfw: Plug memory leak on pc_fw_add_pflash_drv() error path hw/pc_sysfw.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) -- -- 1.7.11.2
pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily
creates a drive without a medium.
When pc_system_flash_init() asks for its size, bdrv_getlength() fails
with -ENOMEDIUM, which isn't checked either. It fails relatively
cleanly only because -ENOMEDIUM isn't a multiple of 4096:
$ qemu-system-x86_64 -S -vnc :0 -bios nonexistant
qemu: PC system firmware (pflash) must be a multiple of 0x1000
[Exit 1 ]
Fix by handling the qemu_find_file() failure.
Signed-off-by: Markus Armbruster <armbru <at> redhat.com>
---
hw/pc_sysfw.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index b45f0ac..fd22154 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
<at> <at> -84,6 +84,11 <at> <at> static void pc_fw_add_pflash_drv(void)
bios_name = BIOS_FILENAME;
}
filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+ if (!filename) {
+ error_report("Can't open BIOS image %s: %s",
+ bios_name, strerror(errno));
+ exit(1);
(Continue reading)
On Thu, 16 Aug 2012 13:41:12 +0200
Markus Armbruster <armbru <at> redhat.com> wrote:
> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily
> creates a drive without a medium.
>
> When pc_system_flash_init() asks for its size, bdrv_getlength() fails
> with -ENOMEDIUM, which isn't checked either. It fails relatively
> cleanly only because -ENOMEDIUM isn't a multiple of 4096:
>
> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant
> qemu: PC system firmware (pflash) must be a multiple of 0x1000
> [Exit 1 ]
>
> Fix by handling the qemu_find_file() failure.
>
> Signed-off-by: Markus Armbruster <armbru <at> redhat.com>
> ---
> hw/pc_sysfw.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index b45f0ac..fd22154 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> <at> <at> -84,6 +84,11 <at> <at> static void pc_fw_add_pflash_drv(void)
> bios_name = BIOS_FILENAME;
> }
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> + if (!filename) {
(Continue reading)
Luiz Capitulino <lcapitulino <at> redhat.com> writes: > On Thu, 16 Aug 2012 13:41:12 +0200 > Markus Armbruster <armbru <at> redhat.com> wrote: > >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily >> creates a drive without a medium. >> >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails >> with -ENOMEDIUM, which isn't checked either. It fails relatively >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: >> >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 >> [Exit 1 ] >> >> Fix by handling the qemu_find_file() failure. >> >> Signed-off-by: Markus Armbruster <armbru <at> redhat.com> >> --- >> hw/pc_sysfw.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c >> index b45f0ac..fd22154 100644 >> --- a/hw/pc_sysfw.c >> +++ b/hw/pc_sysfw.c >> <at> <at> -84,6 +84,11 <at> <at> static void pc_fw_add_pflash_drv(void) >> bios_name = BIOS_FILENAME; >> }(Continue reading)
Am 16.08.2012 15:50, schrieb Markus Armbruster: >> Although I'm not sure it qualifies for hard-freeze... > > I didn't tag my series "for-1.2". I understand that fixes to > not-so-important stuff aren't welcome at this time even when they're > really simple. It's a clear bug fix, easy to understand and low risk, and even rc0 isn't out yet. I think this would still be fine for 1.2. Kevin
On Thu, 16 Aug 2012 15:50:51 +0200 Markus Armbruster <armbru <at> redhat.com> wrote: > Luiz Capitulino <lcapitulino <at> redhat.com> writes: > > > On Thu, 16 Aug 2012 13:41:12 +0200 > > Markus Armbruster <armbru <at> redhat.com> wrote: > > > >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily > >> creates a drive without a medium. > >> > >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails > >> with -ENOMEDIUM, which isn't checked either. It fails relatively > >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: > >> > >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant > >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 > >> [Exit 1 ] > >> > >> Fix by handling the qemu_find_file() failure. > >> > >> Signed-off-by: Markus Armbruster <armbru <at> redhat.com> > >> --- > >> hw/pc_sysfw.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c > >> index b45f0ac..fd22154 100644 > >> --- a/hw/pc_sysfw.c > >> +++ b/hw/pc_sysfw.c(Continue reading)
Luiz Capitulino <lcapitulino <at> redhat.com> writes: > On Thu, 16 Aug 2012 15:50:51 +0200 > Markus Armbruster <armbru <at> redhat.com> wrote: > >> Luiz Capitulino <lcapitulino <at> redhat.com> writes: >> >> > On Thu, 16 Aug 2012 13:41:12 +0200 >> > Markus Armbruster <armbru <at> redhat.com> wrote: >> > >> >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily >> >> creates a drive without a medium. >> >> >> >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails >> >> with -ENOMEDIUM, which isn't checked either. It fails relatively >> >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: >> >> >> >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant >> >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 >> >> [Exit 1 ] >> >> >> >> Fix by handling the qemu_find_file() failure. >> >> >> >> Signed-off-by: Markus Armbruster <armbru <at> redhat.com> >> >> --- >> >> hw/pc_sysfw.c | 5 +++++ >> >> 1 file changed, 5 insertions(+) >> >> >> >> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c >> >> index b45f0ac..fd22154 100644(Continue reading)
On Thu, 16 Aug 2012 16:32:12 +0200 Markus Armbruster <armbru <at> redhat.com> wrote: > Luiz Capitulino <lcapitulino <at> redhat.com> writes: > > > On Thu, 16 Aug 2012 15:50:51 +0200 > > Markus Armbruster <armbru <at> redhat.com> wrote: > > > >> Luiz Capitulino <lcapitulino <at> redhat.com> writes: > >> > >> > On Thu, 16 Aug 2012 13:41:12 +0200 > >> > Markus Armbruster <armbru <at> redhat.com> wrote: > >> > > >> >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily > >> >> creates a drive without a medium. > >> >> > >> >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails > >> >> with -ENOMEDIUM, which isn't checked either. It fails relatively > >> >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: > >> >> > >> >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant > >> >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 > >> >> [Exit 1 ] > >> >> > >> >> Fix by handling the qemu_find_file() failure. > >> >> > >> >> Signed-off-by: Markus Armbruster <armbru <at> redhat.com> > >> >> --- > >> >> hw/pc_sysfw.c | 5 +++++ > >> >> 1 file changed, 5 insertions(+)(Continue reading)
Luiz Capitulino <lcapitulino <at> redhat.com> writes: > On Thu, 16 Aug 2012 16:32:12 +0200 > Markus Armbruster <armbru <at> redhat.com> wrote: > >> Luiz Capitulino <lcapitulino <at> redhat.com> writes: >> >> > On Thu, 16 Aug 2012 15:50:51 +0200 >> > Markus Armbruster <armbru <at> redhat.com> wrote: >> > >> >> Luiz Capitulino <lcapitulino <at> redhat.com> writes: >> >> >> >> > On Thu, 16 Aug 2012 13:41:12 +0200 >> >> > Markus Armbruster <armbru <at> redhat.com> wrote: >> >> > >> >> >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily >> >> >> creates a drive without a medium. >> >> >> >> >> >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails >> >> >> with -ENOMEDIUM, which isn't checked either. It fails relatively >> >> >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: >> >> >> >> >> >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant >> >> >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 >> >> >> [Exit 1 ] >> >> >> >> >> >> Fix by handling the qemu_find_file() failure. >> >> >> >> >> >> Signed-off-by: Markus Armbruster <armbru <at> redhat.com> >> >> >> ---(Continue reading)
On Thu, 16 Aug 2012 17:12:37 +0200 Markus Armbruster <armbru <at> redhat.com> wrote: > Luiz Capitulino <lcapitulino <at> redhat.com> writes: > > > On Thu, 16 Aug 2012 16:32:12 +0200 > > Markus Armbruster <armbru <at> redhat.com> wrote: > > > >> Luiz Capitulino <lcapitulino <at> redhat.com> writes: > >> > >> > On Thu, 16 Aug 2012 15:50:51 +0200 > >> > Markus Armbruster <armbru <at> redhat.com> wrote: > >> > > >> >> Luiz Capitulino <lcapitulino <at> redhat.com> writes: > >> >> > >> >> > On Thu, 16 Aug 2012 13:41:12 +0200 > >> >> > Markus Armbruster <armbru <at> redhat.com> wrote: > >> >> > > >> >> >> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily > >> >> >> creates a drive without a medium. > >> >> >> > >> >> >> When pc_system_flash_init() asks for its size, bdrv_getlength() fails > >> >> >> with -ENOMEDIUM, which isn't checked either. It fails relatively > >> >> >> cleanly only because -ENOMEDIUM isn't a multiple of 4096: > >> >> >> > >> >> >> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant > >> >> >> qemu: PC system firmware (pflash) must be a multiple of 0x1000 > >> >> >> [Exit 1 ] > >> >> >> > >> >> >> Fix by handling the qemu_find_file() failure.(Continue reading)
On Thu, 16 Aug 2012 13:49:15 -0300 Luiz Capitulino <lcapitulino <at> redhat.com> wrote: > > I converted more error messages to the error-reporting-infrastructure- > > du-jour than was enjoyable, and I can tell you that the restrictions > > that come with error_report() compared to anything-goes-fprintf() do > > make the job easier. > > This patch is fixing a function that's only used in command-line context, > I don't see why fprintf() shouldn't be a good fit. Your call about PROGNAME > is a valid one, but no function in the call chain prints it yet, so it's not > a big deal, specially if compared to the alternative (which is using > error_report()). I've talked with Markus about this on IRC and (correct me if I'm wrong Markus), he says that we could kill the HMP stuff from error_report() once no caller is depending on it. Looks like a plan. I'd rather not add new error_report() calls where it's not strictly needed, but no biggie.
Reviewed-by: Jordan Justen <jordan.l.justen <at> intel.com>
On Thu, Aug 16, 2012 at 4:41 AM, Markus Armbruster <armbru <at> redhat.com> wrote:
> pc_fw_add_pflash_drv() ignores qemu_find_file() failure, and happily
> creates a drive without a medium.
>
> When pc_system_flash_init() asks for its size, bdrv_getlength() fails
> with -ENOMEDIUM, which isn't checked either. It fails relatively
> cleanly only because -ENOMEDIUM isn't a multiple of 4096:
>
> $ qemu-system-x86_64 -S -vnc :0 -bios nonexistant
> qemu: PC system firmware (pflash) must be a multiple of 0x1000
> [Exit 1 ]
>
> Fix by handling the qemu_find_file() failure.
>
> Signed-off-by: Markus Armbruster <armbru <at> redhat.com>
> ---
> hw/pc_sysfw.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index b45f0ac..fd22154 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> <at> <at> -84,6 +84,11 <at> <at> static void pc_fw_add_pflash_drv(void)
> bios_name = BIOS_FILENAME;
> }
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> + if (!filename) {
(Continue reading)
Harmless, because we the error inevitably leads to another, fatal one
in pc_system_flash_init(): PC system firmware (pflash) not available.
Fix it anyway.
Signed-off-by: Markus Armbruster <armbru <at> redhat.com>
---
hw/pc_sysfw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index fd22154..ebb6b25 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
<at> <at> -103,7 +103,9 <at> <at> static void pc_fw_add_pflash_drv(void)
return;
}
- drive_init(opts, machine->use_scsi);
+ if (!drive_init(opts, machine->use_scsi)) {
+ qemu_opts_del(opts);
+ }
}
static void pc_system_flash_init(MemoryRegion *rom_memory,
--
--
1.7.11.2
RSS Feed