conf: clear the acpiNodeset field after freeing

[PATCH] conf: clear the acpiNodeset field after freeing

Posted by Daniel P. Berrangé via Devel 3 weeks, 4 days ago

From: Daniel P. Berrangé <berrange@redhat.com>

The virDomainDeviceInfoClear method does not free the struct, only
its contents, so all pointer fields must be explicitly set to NULL
after releasing to avoid disk of double-free.

Reported by coverity:

  *** CID 895678:         Memory - corruptions  (USE_AFTER_FREE)
  /src/conf/domain_conf.c: 5926             in virDomainDeviceInfoParseXML()
  5920             goto cleanup;
  5921
  5922
  5923         ret = 0;
  5924      cleanup:
  5925         if (ret < 0)
  >>>     CID 895678:         Memory - corruptions  (USE_AFTER_FREE)
  >>>     Calling "virDomainDeviceInfoClear" frees pointer "info->acpiNodeset" which has already been freed.
  5926             virDomainDeviceInfoClear(info);
  5927         return ret;
  5928     }
  5929
  5930     static int
  5931     virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/conf/device_conf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index d08de68717..3fa7bba649 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -138,6 +138,7 @@ virDomainDeviceInfoClear(virDomainDeviceInfo *info)
     VIR_FREE(info->romfile);
     VIR_FREE(info->loadparm);
     virBitmapFree(info->acpiNodeset);
+    info->acpiNodeset = NUll;
     info->isolationGroup = 0;
     info->isolationGroupLocked = false;
 }
-- 
2.50.1

Re: [PATCH] conf: clear the acpiNodeset field after freeing

Posted by Peter Krempa via Devel 3 weeks, 4 days ago

On Tue, Sep 09, 2025 at 10:28:33 +0100, Daniel P. Berrangé via Devel wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> The virDomainDeviceInfoClear method does not free the struct, only
> its contents, so all pointer fields must be explicitly set to NULL
> after releasing to avoid disk of double-free.
> 
> Reported by coverity:
> 
>   *** CID 895678:         Memory - corruptions  (USE_AFTER_FREE)
>   /src/conf/domain_conf.c: 5926             in virDomainDeviceInfoParseXML()
>   5920             goto cleanup;
>   5921
>   5922
>   5923         ret = 0;
>   5924      cleanup:
>   5925         if (ret < 0)
>   >>>     CID 895678:         Memory - corruptions  (USE_AFTER_FREE)
>   >>>     Calling "virDomainDeviceInfoClear" frees pointer "info->acpiNodeset" which has already been freed.
>   5926             virDomainDeviceInfoClear(info);
>   5927         return ret;
>   5928     }
>   5929
>   5930     static int
>   5931     virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/conf/device_conf.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index d08de68717..3fa7bba649 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -138,6 +138,7 @@ virDomainDeviceInfoClear(virDomainDeviceInfo *info)
>      VIR_FREE(info->romfile);
>      VIR_FREE(info->loadparm);
>      virBitmapFree(info->acpiNodeset);
> +    info->acpiNodeset = NUll;

NULL instead of NUll

Also consider using g_clear_pointer(&info->acpiNodeset, virBitmapFree)
instead.


Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Re: [PATCH] conf: clear the acpiNodeset field after freeing

Posted by Daniel P. Berrangé via Devel 3 weeks, 4 days ago

On Tue, Sep 09, 2025 at 11:36:42AM +0200, Peter Krempa wrote:
> On Tue, Sep 09, 2025 at 10:28:33 +0100, Daniel P. Berrangé via Devel wrote:
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > The virDomainDeviceInfoClear method does not free the struct, only
> > its contents, so all pointer fields must be explicitly set to NULL
> > after releasing to avoid disk of double-free.
> > 
> > Reported by coverity:
> > 
> >   *** CID 895678:         Memory - corruptions  (USE_AFTER_FREE)
> >   /src/conf/domain_conf.c: 5926             in virDomainDeviceInfoParseXML()
> >   5920             goto cleanup;
> >   5921
> >   5922
> >   5923         ret = 0;
> >   5924      cleanup:
> >   5925         if (ret < 0)
> >   >>>     CID 895678:         Memory - corruptions  (USE_AFTER_FREE)
> >   >>>     Calling "virDomainDeviceInfoClear" frees pointer "info->acpiNodeset" which has already been freed.
> >   5926             virDomainDeviceInfoClear(info);
> >   5927         return ret;
> >   5928     }
> >   5929
> >   5930     static int
> >   5931     virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/conf/device_conf.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > index d08de68717..3fa7bba649 100644
> > --- a/src/conf/device_conf.c
> > +++ b/src/conf/device_conf.c
> > @@ -138,6 +138,7 @@ virDomainDeviceInfoClear(virDomainDeviceInfo *info)
> >      VIR_FREE(info->romfile);
> >      VIR_FREE(info->loadparm);
> >      virBitmapFree(info->acpiNodeset);
> > +    info->acpiNodeset = NUll;
> 
> NULL instead of NUll
> 
> Also consider using g_clear_pointer(&info->acpiNodeset, virBitmapFree)
> instead.

Yes, I've done that instead.

> 
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

With 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 :|