* [PATCH-for-5.1 1/4] qemu/osdep: Document os_find_datadir() return value
2020-07-14 16:42 [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Philippe Mathieu-Daudé
@ 2020-07-14 16:42 ` Philippe Mathieu-Daudé
2020-07-20 14:10 ` Daniel P. Berrangé
2020-07-14 16:42 ` [PATCH-for-5.1 2/4] qemu/osdep: Reword qemu_get_exec_dir() documentation Philippe Mathieu-Daudé
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 16:42 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sarah Harris, Stefan Weil,
Philippe Mathieu-Daudé,
Michael Rolnik, Paolo Bonzini
Document os_find_datadir() returned data must be freed.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
os-posix.c | 3 +++
os-win32.c | 7 ++++++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/os-posix.c b/os-posix.c
index b674b20b1b..3572db3f44 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -84,6 +84,9 @@ void os_setup_signal_handling(void)
* Find a likely location for support files using the location of the binary.
* When running from the build tree this will be "$bindir/../pc-bios".
* Otherwise, this is CONFIG_QEMU_DATADIR.
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
*/
char *os_find_datadir(void)
{
diff --git a/os-win32.c b/os-win32.c
index 6b86e022f0..c9c3afe648 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -57,7 +57,12 @@ void os_setup_early_signal_handling(void)
atexit(os_undo_timer_resolution);
}
-/* Look for support files in the same directory as the executable. */
+/*
+ * Look for support files in the same directory as the executable.
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
+ */
char *os_find_datadir(void)
{
return qemu_get_exec_dir();
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH-for-5.1 1/4] qemu/osdep: Document os_find_datadir() return value
2020-07-14 16:42 ` [PATCH-for-5.1 1/4] qemu/osdep: Document os_find_datadir() return value Philippe Mathieu-Daudé
@ 2020-07-20 14:10 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2020-07-20 14:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, qemu-devel,
Michael Rolnik, Paolo Bonzini
On Tue, Jul 14, 2020 at 06:42:54PM +0200, Philippe Mathieu-Daudé wrote:
> Document os_find_datadir() returned data must be freed.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> os-posix.c | 3 +++
> os-win32.c | 7 ++++++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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
* [PATCH-for-5.1 2/4] qemu/osdep: Reword qemu_get_exec_dir() documentation
2020-07-14 16:42 [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Philippe Mathieu-Daudé
2020-07-14 16:42 ` [PATCH-for-5.1 1/4] qemu/osdep: Document os_find_datadir() return value Philippe Mathieu-Daudé
@ 2020-07-14 16:42 ` Philippe Mathieu-Daudé
2020-07-20 14:10 ` Daniel P. Berrangé
2020-07-14 16:42 ` [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file() Philippe Mathieu-Daudé
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 16:42 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sarah Harris, Stefan Weil,
Philippe Mathieu-Daudé,
Michael Rolnik, Paolo Bonzini
This comment is confuse, reword it a bit.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/qemu/osdep.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 979a403984..a96849dd90 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -588,7 +588,10 @@ char *qemu_get_local_state_pathname(const char *relative_pathname);
void qemu_init_exec_dir(const char *argv0);
/* Get the saved exec dir.
- * Caller needs to release the returned string by g_free() */
+ *
+ * The caller is responsible for releasing the value returned with g_free()
+ * after use.
+ */
char *qemu_get_exec_dir(void);
/**
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH-for-5.1 2/4] qemu/osdep: Reword qemu_get_exec_dir() documentation
2020-07-14 16:42 ` [PATCH-for-5.1 2/4] qemu/osdep: Reword qemu_get_exec_dir() documentation Philippe Mathieu-Daudé
@ 2020-07-20 14:10 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2020-07-20 14:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, qemu-devel,
Michael Rolnik, Paolo Bonzini
On Tue, Jul 14, 2020 at 06:42:55PM +0200, Philippe Mathieu-Daudé wrote:
> This comment is confuse, reword it a bit.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/qemu/osdep.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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
* [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file()
2020-07-14 16:42 [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Philippe Mathieu-Daudé
2020-07-14 16:42 ` [PATCH-for-5.1 1/4] qemu/osdep: Document os_find_datadir() return value Philippe Mathieu-Daudé
2020-07-14 16:42 ` [PATCH-for-5.1 2/4] qemu/osdep: Reword qemu_get_exec_dir() documentation Philippe Mathieu-Daudé
@ 2020-07-14 16:42 ` Philippe Mathieu-Daudé
2020-07-14 18:41 ` Peter Maydell
` (2 more replies)
2020-07-14 16:42 ` [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware() Philippe Mathieu-Daudé
` (2 subsequent siblings)
5 siblings, 3 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 16:42 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sarah Harris, Stefan Weil,
Philippe Mathieu-Daudé,
Michael Rolnik, Paolo Bonzini
Document qemu_find_file(), in particular the returned
value which must be freed.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/qemu-common.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index d0142f29ac..d6a08259d3 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -110,6 +110,20 @@ const char *qemu_get_vm_name(void);
#define QEMU_FILE_TYPE_BIOS 0
#define QEMU_FILE_TYPE_KEYMAP 1
+/**
+ * qemu_find_file:
+ * @type: QEMU_FILE_TYPE_BIOS (for BIOS, VGA BIOS)
+ * or QEMU_FILE_TYPE_KEYMAP (for keymaps).
+ * @name: File name
+ *
+ * Search for @name file in the data directories, either configured at
+ * build time (DATADIR) or registered with the -L command line option.
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
+ *
+ * Returns: absolute path to the file or NULL on error.
+ */
char *qemu_find_file(int type, const char *name);
/* OS specific functions */
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file()
2020-07-14 16:42 ` [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file() Philippe Mathieu-Daudé
@ 2020-07-14 18:41 ` Peter Maydell
2020-07-15 1:04 ` Li Qiang
2020-07-20 14:16 ` Daniel P. Berrangé
2 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2020-07-14 18:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Sarah Harris, Michael Rolnik, QEMU Developers,
Stefan Weil
On Tue, 14 Jul 2020 at 17:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Document qemu_find_file(), in particular the returned
> value which must be freed.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/qemu-common.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d0142f29ac..d6a08259d3 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -110,6 +110,20 @@ const char *qemu_get_vm_name(void);
>
> #define QEMU_FILE_TYPE_BIOS 0
> #define QEMU_FILE_TYPE_KEYMAP 1
> +/**
> + * qemu_find_file:
> + * @type: QEMU_FILE_TYPE_BIOS (for BIOS, VGA BIOS)
> + * or QEMU_FILE_TYPE_KEYMAP (for keymaps).
> + * @name: File name
> + *
> + * Search for @name file in the data directories, either configured at
> + * build time (DATADIR) or registered with the -L command line option.
> + *
> + * The caller must use g_free() to free the returned data when it is
> + * no longer required.
> + *
> + * Returns: absolute path to the file or NULL on error.
> + */
> char *qemu_find_file(int type, const char *name);
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file()
2020-07-14 16:42 ` [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file() Philippe Mathieu-Daudé
2020-07-14 18:41 ` Peter Maydell
@ 2020-07-15 1:04 ` Li Qiang
2020-07-20 14:16 ` Daniel P. Berrangé
2 siblings, 0 replies; 17+ messages in thread
From: Li Qiang @ 2020-07-15 1:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, Qemu Developers,
Michael Rolnik, Paolo Bonzini
Philippe Mathieu-Daudé <f4bug@amsat.org> 于2020年7月15日周三 上午12:48写道：
>
> Document qemu_find_file(), in particular the returned
> value which must be freed.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
> ---
> include/qemu-common.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d0142f29ac..d6a08259d3 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -110,6 +110,20 @@ const char *qemu_get_vm_name(void);
>
> #define QEMU_FILE_TYPE_BIOS 0
> #define QEMU_FILE_TYPE_KEYMAP 1
> +/**
> + * qemu_find_file:
> + * @type: QEMU_FILE_TYPE_BIOS (for BIOS, VGA BIOS)
> + * or QEMU_FILE_TYPE_KEYMAP (for keymaps).
> + * @name: File name
> + *
> + * Search for @name file in the data directories, either configured at
> + * build time (DATADIR) or registered with the -L command line option.
> + *
> + * The caller must use g_free() to free the returned data when it is
> + * no longer required.
> + *
> + * Returns: absolute path to the file or NULL on error.
> + */
> char *qemu_find_file(int type, const char *name);
>
> /* OS specific functions */
> --
> 2.21.3
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file()
2020-07-14 16:42 ` [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file() Philippe Mathieu-Daudé
2020-07-14 18:41 ` Peter Maydell
2020-07-15 1:04 ` Li Qiang
@ 2020-07-20 14:16 ` Daniel P. Berrangé
2020-07-20 18:43 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2020-07-20 14:16 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, qemu-devel,
Michael Rolnik, Paolo Bonzini
On Tue, Jul 14, 2020 at 06:42:56PM +0200, Philippe Mathieu-Daudé wrote:
> Document qemu_find_file(), in particular the returned
> value which must be freed.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> include/qemu-common.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d0142f29ac..d6a08259d3 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -110,6 +110,20 @@ const char *qemu_get_vm_name(void);
>
> #define QEMU_FILE_TYPE_BIOS 0
> #define QEMU_FILE_TYPE_KEYMAP 1
> +/**
> + * qemu_find_file:
> + * @type: QEMU_FILE_TYPE_BIOS (for BIOS, VGA BIOS)
> + * or QEMU_FILE_TYPE_KEYMAP (for keymaps).
> + * @name: File name
s/File name/relative or absolute file name/
> + *
Insert:
"If @name exists on disk as an absolute path, or a path relative
to the current directory, then returns @name unchanged.
> + * Search for @name file in the data directories, either configured at
> + * build time (DATADIR) or registered with the -L command line option.
s/Search/Otherwise searches/
> + *
> + * The caller must use g_free() to free the returned data when it is
> + * no longer required.
> + *
> + * Returns: absolute path to the file or NULL on error.
It doesn't always return an absolute path. It can return a path
relative to the currently working directory via the initial
short-circuit access() check.
Returns: a path that can access @name, or NULL if no matching
file exists.
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: [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file()
2020-07-20 14:16 ` Daniel P. Berrangé
@ 2020-07-20 18:43 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-20 18:43 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, qemu-devel,
Michael Rolnik, Paolo Bonzini
On 7/20/20 4:16 PM, Daniel P. Berrangé wrote:
> On Tue, Jul 14, 2020 at 06:42:56PM +0200, Philippe Mathieu-Daudé wrote:
>> Document qemu_find_file(), in particular the returned
>> value which must be freed.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/qemu-common.h | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index d0142f29ac..d6a08259d3 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -110,6 +110,20 @@ const char *qemu_get_vm_name(void);
>>
>> #define QEMU_FILE_TYPE_BIOS 0
>> #define QEMU_FILE_TYPE_KEYMAP 1
>> +/**
>> + * qemu_find_file:
>> + * @type: QEMU_FILE_TYPE_BIOS (for BIOS, VGA BIOS)
>> + * or QEMU_FILE_TYPE_KEYMAP (for keymaps).
>> + * @name: File name
>
> s/File name/relative or absolute file name/
>
>> + *
>
> Insert:
>
> "If @name exists on disk as an absolute path, or a path relative
> to the current directory, then returns @name unchanged.
>
>
>> + * Search for @name file in the data directories, either configured at
>> + * build time (DATADIR) or registered with the -L command line option.
>
> s/Search/Otherwise searches/
>
>
>> + *
>> + * The caller must use g_free() to free the returned data when it is
>> + * no longer required.
>> + *
>> + * Returns: absolute path to the file or NULL on error.
>
> It doesn't always return an absolute path. It can return a path
> relative to the currently working directory via the initial
> short-circuit access() check.
>
> Returns: a path that can access @name, or NULL if no matching
> file exists.
>
Thanks for the corrections!
>
> Regards,
> Daniel
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware()
2020-07-14 16:42 [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2020-07-14 16:42 ` [PATCH-for-5.1 3/4] qemu-common: Document qemu_find_file() Philippe Mathieu-Daudé
@ 2020-07-14 16:42 ` Philippe Mathieu-Daudé
2020-07-20 12:39 ` Philippe Mathieu-Daudé
2020-07-20 14:18 ` Daniel P. Berrangé
2020-07-20 13:24 ` [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Michael Rolnik
2020-07-20 18:48 ` Philippe Mathieu-Daudé
5 siblings, 2 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-14 16:42 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sarah Harris, Stefan Weil,
Philippe Mathieu-Daudé,
Michael Rolnik, Paolo Bonzini
The value returned by qemu_find_file() must be freed.
This fixes Coverity issue CID 1430449, which points out
that the memory returned by qemu_find_file() is leaked.
Fixes: Coverity CID 1430449 (RESOURCE_LEAK)
Fixes: 7dd8f6fde4 ('hw/avr: Add support for loading ELF/raw binaries')
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/avr/boot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/avr/boot.c b/hw/avr/boot.c
index 6fbcde4061..151734f82d 100644
--- a/hw/avr/boot.c
+++ b/hw/avr/boot.c
@@ -60,7 +60,7 @@ static const char *avr_elf_e_flags_to_cpu_type(uint32_t flags)
bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
MemoryRegion *program_mr, const char *firmware)
{
- const char *filename;
+ g_autofree char *filename;
int bytes_loaded;
uint64_t entry;
uint32_t e_flags;
--
2.21.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware()
2020-07-14 16:42 ` [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware() Philippe Mathieu-Daudé
@ 2020-07-20 12:39 ` Philippe Mathieu-Daudé
2020-07-20 14:18 ` Daniel P. Berrangé
1 sibling, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-20 12:39 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sarah Harris, Paolo Bonzini, Michael Rolnik, Stefan Weil
ping?
On 7/14/20 6:42 PM, Philippe Mathieu-Daudé wrote:
> The value returned by qemu_find_file() must be freed.
>
> This fixes Coverity issue CID 1430449, which points out
> that the memory returned by qemu_find_file() is leaked.
>
> Fixes: Coverity CID 1430449 (RESOURCE_LEAK)
> Fixes: 7dd8f6fde4 ('hw/avr: Add support for loading ELF/raw binaries')
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/avr/boot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/avr/boot.c b/hw/avr/boot.c
> index 6fbcde4061..151734f82d 100644
> --- a/hw/avr/boot.c
> +++ b/hw/avr/boot.c
> @@ -60,7 +60,7 @@ static const char *avr_elf_e_flags_to_cpu_type(uint32_t flags)
> bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
> MemoryRegion *program_mr, const char *firmware)
> {
> - const char *filename;
> + g_autofree char *filename;
> int bytes_loaded;
> uint64_t entry;
> uint32_t e_flags;
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware()
2020-07-14 16:42 ` [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware() Philippe Mathieu-Daudé
2020-07-20 12:39 ` Philippe Mathieu-Daudé
@ 2020-07-20 14:18 ` Daniel P. Berrangé
2020-07-20 18:45 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2020-07-20 14:18 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, qemu-devel,
Michael Rolnik, Paolo Bonzini
On Tue, Jul 14, 2020 at 06:42:57PM +0200, Philippe Mathieu-Daudé wrote:
> The value returned by qemu_find_file() must be freed.
>
> This fixes Coverity issue CID 1430449, which points out
> that the memory returned by qemu_find_file() is leaked.
>
> Fixes: Coverity CID 1430449 (RESOURCE_LEAK)
> Fixes: 7dd8f6fde4 ('hw/avr: Add support for loading ELF/raw binaries')
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/avr/boot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/avr/boot.c b/hw/avr/boot.c
> index 6fbcde4061..151734f82d 100644
> --- a/hw/avr/boot.c
> +++ b/hw/avr/boot.c
> @@ -60,7 +60,7 @@ static const char *avr_elf_e_flags_to_cpu_type(uint32_t flags)
> bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
> MemoryRegion *program_mr, const char *firmware)
> {
> - const char *filename;
> + g_autofree char *filename;
Any variable marked g_autofree or g_auto must always be initialized
to NULL otherwise there's risk of free'ing uninitialized data. Even
if currently safe, any later refactoring could turn it into a bug.
So iff "= NULL" is added:
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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: [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware()
2020-07-20 14:18 ` Daniel P. Berrangé
@ 2020-07-20 18:45 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-20 18:45 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Maydell, Sarah Harris, Stefan Weil, qemu-devel,
Michael Rolnik, Paolo Bonzini
On 7/20/20 4:18 PM, Daniel P. Berrangé wrote:
> On Tue, Jul 14, 2020 at 06:42:57PM +0200, Philippe Mathieu-Daudé wrote:
>> The value returned by qemu_find_file() must be freed.
>>
>> This fixes Coverity issue CID 1430449, which points out
>> that the memory returned by qemu_find_file() is leaked.
>>
>> Fixes: Coverity CID 1430449 (RESOURCE_LEAK)
>> Fixes: 7dd8f6fde4 ('hw/avr: Add support for loading ELF/raw binaries')
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/avr/boot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/avr/boot.c b/hw/avr/boot.c
>> index 6fbcde4061..151734f82d 100644
>> --- a/hw/avr/boot.c
>> +++ b/hw/avr/boot.c
>> @@ -60,7 +60,7 @@ static const char *avr_elf_e_flags_to_cpu_type(uint32_t flags)
>> bool avr_load_firmware(AVRCPU *cpu, MachineState *ms,
>> MemoryRegion *program_mr, const char *firmware)
>> {
>> - const char *filename;
>> + g_autofree char *filename;
>
> Any variable marked g_autofree or g_auto must always be initialized
> to NULL otherwise there's risk of free'ing uninitialized data. Even
> if currently safe, any later refactoring could turn it into a bug.
TIL, thanks :)
>
> So iff "= NULL" is added:
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> Regards,
> Daniel
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware
2020-07-14 16:42 [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2020-07-14 16:42 ` [PATCH-for-5.1 4/4] hw/avr/boot: Fix memory leak in avr_load_firmware() Philippe Mathieu-Daudé
@ 2020-07-20 13:24 ` Michael Rolnik
2020-07-20 13:38 ` Michael Rolnik
2020-07-20 18:48 ` Philippe Mathieu-Daudé
5 siblings, 1 reply; 17+ messages in thread
From: Michael Rolnik @ 2020-07-20 13:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Paolo Bonzini, Sarah Harris, QEMU Developers, Stefan Weil
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
On Tue, Jul 14, 2020 at 7:42 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:
> Fix the memory leak reported by Coverity (CID 1430449).
>
> Philippe Mathieu-Daudé (4):
> qemu/osdep: Document os_find_datadir() return value
> qemu/osdep: Reword qemu_get_exec_dir() documentation
> qemu-common: Document qemu_find_file()
> hw/avr/boot: Fix memory leak in avr_load_firmware()
>
> include/qemu-common.h | 14 ++++++++++++++
> include/qemu/osdep.h | 5 ++++-
> hw/avr/boot.c | 2 +-
> os-posix.c | 3 +++
> os-win32.c | 7 ++++++-
> 5 files changed, 28 insertions(+), 3 deletions(-)
>
> --
> 2.21.3
>
>
--
Best Regards,
Michael Rolnik
[-- Attachment #2: Type: text/html, Size: 1297 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware
2020-07-20 13:24 ` [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Michael Rolnik
@ 2020-07-20 13:38 ` Michael Rolnik
0 siblings, 0 replies; 17+ messages in thread
From: Michael Rolnik @ 2020-07-20 13:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Paolo Bonzini, Sarah Harris, QEMU Developers, Stefan Weil
[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]
Tested-by: Michael Rolnik <mrolnik@gmail.com>
I mean I got the patch, built and then ran *make check-qtest-avr* and *make
check-acceptance*
On Mon, Jul 20, 2020 at 4:24 PM Michael Rolnik <mrolnik@gmail.com> wrote:
> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
>
>
> On Tue, Jul 14, 2020 at 7:42 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>> Fix the memory leak reported by Coverity (CID 1430449).
>>
>> Philippe Mathieu-Daudé (4):
>> qemu/osdep: Document os_find_datadir() return value
>> qemu/osdep: Reword qemu_get_exec_dir() documentation
>> qemu-common: Document qemu_find_file()
>> hw/avr/boot: Fix memory leak in avr_load_firmware()
>>
>> include/qemu-common.h | 14 ++++++++++++++
>> include/qemu/osdep.h | 5 ++++-
>> hw/avr/boot.c | 2 +-
>> os-posix.c | 3 +++
>> os-win32.c | 7 ++++++-
>> 5 files changed, 28 insertions(+), 3 deletions(-)
>>
>> --
>> 2.21.3
>>
>>
>
> --
> Best Regards,
> Michael Rolnik
>
--
Best Regards,
Michael Rolnik
[-- Attachment #2: Type: text/html, Size: 2040 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware
2020-07-14 16:42 [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Philippe Mathieu-Daudé
` (4 preceding siblings ...)
2020-07-20 13:24 ` [PATCH-for-5.1 0/4] misc: Document qemu_find_file and fix memory leak in avr_load_firmware Michael Rolnik
@ 2020-07-20 18:48 ` Philippe Mathieu-Daudé
5 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-20 18:48 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Sarah Harris, Paolo Bonzini, Michael Rolnik, Stefan Weil
On 7/14/20 6:42 PM, Philippe Mathieu-Daudé wrote:
> Fix the memory leak reported by Coverity (CID 1430449).
>
> Philippe Mathieu-Daudé (4):
> qemu/osdep: Document os_find_datadir() return value
> qemu/osdep: Reword qemu_get_exec_dir() documentation
> qemu-common: Document qemu_find_file()
> hw/avr/boot: Fix memory leak in avr_load_firmware()
>
> include/qemu-common.h | 14 ++++++++++++++
> include/qemu/osdep.h | 5 ++++-
> hw/avr/boot.c | 2 +-
> os-posix.c | 3 +++
> os-win32.c | 7 ++++++-
> 5 files changed, 28 insertions(+), 3 deletions(-)
>
Thanks to the reviewers.
I addressed Daniel's review comments and will send a pull
request once the testing succeeds.
Phil.
^ permalink raw reply [flat|nested] 17+ messages in thread