Re: [PATCH] vfio/iommufd: Fix memory leak

[Cédric Le Goater]

On 3/14/24 04:31, Duan, Zhenzhong wrote:




-Original Message-
From: Cédric Le Goater 
Sent: Thursday, March 14, 2024 5:06 AM
To: qemu-devel@nongnu.org
Cc: Alex Williamson ; Cédric Le Goater
; Eric Auger ; Liu, Yi L
; Duan, Zhenzhong 
Subject: [PATCH] vfio/iommufd: Fix memory leak

Make sure variable contents is freed if scanf fails.

Cc: Eric Auger 
Cc: Yi Liu 
Cc: Zhenzhong Duan 
Fixes: CID 1540007
Fixes: 5ee3dc7af785 ("vfio/iommufd: Implement the iommufd backend")
Signed-off-by: Cédric Le Goater 


Reviewed-by: Zhenzhong Duan 

Unrelated to this patch, I see there are four g_free calls, not clear if it's 
deserved
to cleanup with g_autofree.


Ah yes. This is much better indeed.


Thanks,

C.

RE: [PATCH] vfio/iommufd: Fix memory leak

[Duan, Zhenzhong]

>-Original Message-
>From: Cédric Le Goater 
>Sent: Thursday, March 14, 2024 5:06 AM
>To: qemu-devel@nongnu.org
>Cc: Alex Williamson ; Cédric Le Goater
>; Eric Auger ; Liu, Yi L
>; Duan, Zhenzhong 
>Subject: [PATCH] vfio/iommufd: Fix memory leak
>
>Make sure variable contents is freed if scanf fails.
>
>Cc: Eric Auger 
>Cc: Yi Liu 
>Cc: Zhenzhong Duan 
>Fixes: CID 1540007
>Fixes: 5ee3dc7af785 ("vfio/iommufd: Implement the iommufd backend")
>Signed-off-by: Cédric Le Goater 

Reviewed-by: Zhenzhong Duan 

Unrelated to this patch, I see there are four g_free calls, not clear if it's 
deserved
to cleanup with g_autofree.

Thanks
Zhenzhong

>---
> hw/vfio/iommufd.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>index
>a75a785e90c64cdcc4d10c88d217801b3f536cdb..cd549e0ee8573e75772c5
>1cc96153762a6bc8550 100644
>--- a/hw/vfio/iommufd.c
>+++ b/hw/vfio/iommufd.c
>@@ -152,9 +152,8 @@ static int iommufd_cdev_getfd(const char
>*sysfs_path, Error **errp)
>
> if (sscanf(contents, "%d:%d", , ) != 2) {
> error_setg(errp, "failed to get major:minor for \"%s\"", 
> vfio_dev_path);
>-goto out_free_dev_path;
>+goto out_free_contents;
> }
>-g_free(contents);
> vfio_devt = makedev(major, minor);
>
> vfio_path = g_strdup_printf("/dev/vfio/devices/%s", dent->d_name);
>@@ -166,6 +165,8 @@ static int iommufd_cdev_getfd(const char
>*sysfs_path, Error **errp)
> trace_iommufd_cdev_getfd(vfio_path, ret);
> g_free(vfio_path);
>
>+out_free_contents:
>+g_free(contents);
> out_free_dev_path:
> g_free(vfio_dev_path);
> out_close_dir:
>--
>2.44.0

Re: [PATCH] vfio/iommufd: Fix memory leak

[Yi Liu]

On 2024/3/14 05:06, Cédric Le Goater wrote:

Make sure variable contents is freed if scanf fails.

Cc: Eric Auger 
Cc: Yi Liu 
Cc: Zhenzhong Duan 
Fixes: CID 1540007
Fixes: 5ee3dc7af785 ("vfio/iommufd: Implement the iommufd backend")
Signed-off-by: Cédric Le Goater 
---
  hw/vfio/iommufd.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 
a75a785e90c64cdcc4d10c88d217801b3f536cdb..cd549e0ee8573e75772c51cc96153762a6bc8550
 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -152,9 +152,8 @@ static int iommufd_cdev_getfd(const char *sysfs_path, Error 
**errp)
  
  if (sscanf(contents, "%d:%d", , ) != 2) {

  error_setg(errp, "failed to get major:minor for \"%s\"", 
vfio_dev_path);
-goto out_free_dev_path;
+goto out_free_contents;
  }
-g_free(contents);
  vfio_devt = makedev(major, minor);
  
  vfio_path = g_strdup_printf("/dev/vfio/devices/%s", dent->d_name);

@@ -166,6 +165,8 @@ static int iommufd_cdev_getfd(const char *sysfs_path, Error 
**errp)
  trace_iommufd_cdev_getfd(vfio_path, ret);
  g_free(vfio_path);
  
+out_free_contents:

+g_free(contents);
  out_free_dev_path:
  g_free(vfio_dev_path);
  out_close_dir:


good catch.

Reviewed-by: Yi Liu 

--
Regards,
Yi Liu

Re: [PATCH] vfio/iommufd: Fix memory leak

[Eric Auger]

On 3/13/24 22:06, Cédric Le Goater wrote:
> Make sure variable contents is freed if scanf fails.
>
> Cc: Eric Auger 
> Cc: Yi Liu 
> Cc: Zhenzhong Duan 
> Fixes: CID 1540007
> Fixes: 5ee3dc7af785 ("vfio/iommufd: Implement the iommufd backend")
> Signed-off-by: Cédric Le Goater 
Reviewed-by: Eric Auger 

Thanks!

Eric
> ---
>  hw/vfio/iommufd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 
> a75a785e90c64cdcc4d10c88d217801b3f536cdb..cd549e0ee8573e75772c51cc96153762a6bc8550
>  100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -152,9 +152,8 @@ static int iommufd_cdev_getfd(const char *sysfs_path, 
> Error **errp)
>  
>  if (sscanf(contents, "%d:%d", , ) != 2) {
>  error_setg(errp, "failed to get major:minor for \"%s\"", 
> vfio_dev_path);
> -goto out_free_dev_path;
> +goto out_free_contents;
>  }
> -g_free(contents);
>  vfio_devt = makedev(major, minor);
>  
>  vfio_path = g_strdup_printf("/dev/vfio/devices/%s", dent->d_name);
> @@ -166,6 +165,8 @@ static int iommufd_cdev_getfd(const char *sysfs_path, 
> Error **errp)
>  trace_iommufd_cdev_getfd(vfio_path, ret);
>  g_free(vfio_path);
>  
> +out_free_contents:
> +g_free(contents);
>  out_free_dev_path:
>  g_free(vfio_dev_path);
>  out_close_dir: