diff mbox series

[v2,12/18] hw/nvram/fw_cfg: Keep reference of file_data in FWCfgState

Message ID 20190308013222.12524-13-philmd@redhat.com
State New
Headers show
Series fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy | expand

Commit Message

Philippe Mathieu-Daudé March 8, 2019, 1:32 a.m. UTC
The 'file_data' is allocated by read_splashfile() (introduced in
commit 3d3b8303c6f8).  It is then used by fw_cfg_add_file(). Due
to the contract interface of fw_cfg_add_file(), it has to be valid
for the lifetime of the FwCfg object.

Keep a reference of 'file_data' in FWCfgState to be able to
free this memory in fw_cfg_common_unrealize().
We can now remove the res_free() from the main() loop.
The global boot_splash_filedata is now unused, remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c         | 10 ++++++----
 include/hw/nvram/fw_cfg.h |  1 +
 include/sysemu/sysemu.h   |  1 -
 vl.c                      |  9 ---------
 4 files changed, 7 insertions(+), 14 deletions(-)

Comments

Thomas Huth March 8, 2019, 7:02 a.m. UTC | #1
On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
> The 'file_data' is allocated by read_splashfile() (introduced in
> commit 3d3b8303c6f8).  It is then used by fw_cfg_add_file(). Due
> to the contract interface of fw_cfg_add_file(), it has to be valid
> for the lifetime of the FwCfg object.
> 
> Keep a reference of 'file_data' in FWCfgState to be able to
> free this memory in fw_cfg_common_unrealize().
> We can now remove the res_free() from the main() loop.
> The global boot_splash_filedata is now unused, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 10 ++++++----
>  include/hw/nvram/fw_cfg.h |  1 +
>  include/sysemu/sysemu.h   |  1 -
>  vl.c                      |  9 ---------
>  4 files changed, 7 insertions(+), 14 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>
Laszlo Ersek March 8, 2019, 11:16 a.m. UTC | #2
On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> The 'file_data' is allocated by read_splashfile() (introduced in
> commit 3d3b8303c6f8).  It is then used by fw_cfg_add_file(). Due
> to the contract interface of fw_cfg_add_file(), it has to be valid
> for the lifetime of the FwCfg object.
> 
> Keep a reference of 'file_data' in FWCfgState to be able to
> free this memory in fw_cfg_common_unrealize().
> We can now remove the res_free() from the main() loop.
> The global boot_splash_filedata is now unused, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 10 ++++++----
>  include/hw/nvram/fw_cfg.h |  1 +
>  include/sysemu/sysemu.h   |  1 -
>  vl.c                      |  9 ---------
>  4 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3ac6687a04..fc392cb7e0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -215,16 +215,16 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>              g_free(filename);
>              return;
>          }
> -        g_free(boot_splash_filedata);
> -        boot_splash_filedata = (uint8_t *)file_data;
> +        g_free(s->boot_splash.file_data);
> +        s->boot_splash.file_data = file_data;
>  
>          /* insert data */
>          if (file_type == JPG_FILE) {
>              fw_cfg_add_file(s, "bootsplash.jpg",
> -                            boot_splash_filedata, file_size);
> +                            s->boot_splash.file_data, file_size);
>          } else {
>              fw_cfg_add_file(s, "bootsplash.bmp",
> -                            boot_splash_filedata, file_size);
> +                            s->boot_splash.file_data, file_size);
>          }
>          g_free(filename);
>      }
> @@ -974,6 +974,8 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
>  
>      g_free(s->files);
>  
> +    g_free(s->boot_splash.file_data);
> +
>      g_free(s->entries[0]);
>      g_free(s->entries[1]);
>      g_free(s->entry_order);
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index fcb771186c..83a0540b6c 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -56,6 +56,7 @@ struct FWCfgState {
>  
>      uint32_t reboot_timeout;
>      struct {
> +        char *file_data;
>          uint16_t time_le16;
>      } boot_splash;
>  };
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6065d9e420..3cd856b015 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -109,7 +109,6 @@ extern int no_shutdown;
>  extern int old_param;
>  extern int boot_menu;
>  extern bool boot_strict;
> -extern uint8_t *boot_splash_filedata;
>  extern bool enable_mlock;
>  extern bool enable_cpu_pm;
>  extern QEMUClockType rtc_clock;
> diff --git a/vl.c b/vl.c
> index fad6fec38c..47dd63a309 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -187,7 +187,6 @@ unsigned int nb_prom_envs = 0;
>  const char *prom_envs[MAX_PROM_ENVS];
>  int boot_menu;
>  bool boot_strict;
> -uint8_t *boot_splash_filedata;
>  bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
> @@ -558,12 +557,6 @@ const char *qemu_get_vm_name(void)
>      return qemu_name;
>  }
>  
> -static void res_free(void)
> -{
> -    g_free(boot_splash_filedata);
> -    boot_splash_filedata = NULL;
> -}
> -
>  static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      const char *driver = qemu_opt_get(opts, "driver");
> @@ -4591,8 +4584,6 @@ int main(int argc, char **argv, char **envp)
>      job_cancel_sync_all();
>      bdrv_close_all();
>  
> -    res_free();
> -
>      /* vhost-user must be cleaned up before chardevs.  */
>      tpm_cleanup();
>      net_cleanup();
> 

Referring to the earlier thread

  [Qemu-devel] [PATCH] hw/nvram/fw_cfg: Move boot_splash_filedata
                       variables into fw_cfg.c
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg599282.html

my opinion is that *all* of the g_free() calls touched in this patch are
presently -- that is, pre-patch -- bogus:

- As I wrote earlier, res_free() may be reached, but the freeing it does
is useless.

- Furthermore, the g_free() call in fw_cfg_bootsplash() never frees
anything in reality. It is only called from fw_cfg_common_realize(), and
we only have one fw_cfg object (which is never destructed, for now).

So, first I would kill these bogus g_free()s altogether, in a separate
patch (in the separate series that I've recommended elsewhere). Then, in
a second patch (in the separate series), I would include the boot splash
image among the dynamically tracked allocations. Just add it to a linked
list, and when the fw_cfg object is destroyed, release it with the rest.

(Another note (and I should have made it earlier): fw_cfg_modify_i16()
will face a challenge; it will have to update the tracker data structure
too.)

Thanks
Laszlo
diff mbox series

Patch

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3ac6687a04..fc392cb7e0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -215,16 +215,16 @@  static void fw_cfg_bootsplash(FWCfgState *s)
             g_free(filename);
             return;
         }
-        g_free(boot_splash_filedata);
-        boot_splash_filedata = (uint8_t *)file_data;
+        g_free(s->boot_splash.file_data);
+        s->boot_splash.file_data = file_data;
 
         /* insert data */
         if (file_type == JPG_FILE) {
             fw_cfg_add_file(s, "bootsplash.jpg",
-                            boot_splash_filedata, file_size);
+                            s->boot_splash.file_data, file_size);
         } else {
             fw_cfg_add_file(s, "bootsplash.bmp",
-                            boot_splash_filedata, file_size);
+                            s->boot_splash.file_data, file_size);
         }
         g_free(filename);
     }
@@ -974,6 +974,8 @@  static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
 
     g_free(s->files);
 
+    g_free(s->boot_splash.file_data);
+
     g_free(s->entries[0]);
     g_free(s->entries[1]);
     g_free(s->entry_order);
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index fcb771186c..83a0540b6c 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -56,6 +56,7 @@  struct FWCfgState {
 
     uint32_t reboot_timeout;
     struct {
+        char *file_data;
         uint16_t time_le16;
     } boot_splash;
 };
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6065d9e420..3cd856b015 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -109,7 +109,6 @@  extern int no_shutdown;
 extern int old_param;
 extern int boot_menu;
 extern bool boot_strict;
-extern uint8_t *boot_splash_filedata;
 extern bool enable_mlock;
 extern bool enable_cpu_pm;
 extern QEMUClockType rtc_clock;
diff --git a/vl.c b/vl.c
index fad6fec38c..47dd63a309 100644
--- a/vl.c
+++ b/vl.c
@@ -187,7 +187,6 @@  unsigned int nb_prom_envs = 0;
 const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
 bool boot_strict;
-uint8_t *boot_splash_filedata;
 bool wakeup_suspend_enabled;
 
 int icount_align_option;
@@ -558,12 +557,6 @@  const char *qemu_get_vm_name(void)
     return qemu_name;
 }
 
-static void res_free(void)
-{
-    g_free(boot_splash_filedata);
-    boot_splash_filedata = NULL;
-}
-
 static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
 {
     const char *driver = qemu_opt_get(opts, "driver");
@@ -4591,8 +4584,6 @@  int main(int argc, char **argv, char **envp)
     job_cancel_sync_all();
     bdrv_close_all();
 
-    res_free();
-
     /* vhost-user must be cleaned up before chardevs.  */
     tpm_cleanup();
     net_cleanup();