The virNetworkObjSetFwRemoval() function is called at least two
times when there's a network running and network driver
initializes:

1) when loading state XMLs:
  #0  virNetworkObjSetFwRemoval (obj=0x7fffd4028250, fwRemoval=0x7fffd4020ad0) at ../src/conf/virnetworkobj.c:258
  #1  0x00007ffff7a69c68 in virNetworkLoadState (...) at ../src/conf/virnetworkobj.c:952
  #2  0x00007ffff7a6a35d in virNetworkObjLoadAllState (...) at ../src/conf/virnetworkobj.c:1072
  #3  0x00007ffff7f9625f in networkStateInitialize (...) at ../src/network/bridge_driver.c:624

2) when firewall rules are being reloaded:
  #0  virNetworkObjSetFwRemoval (obj=0x7fffd4028250, fwRemoval=0x7fffd402e5b0) at ../src/conf/virnetworkobj.c:258
  #1  0x00007ffff7f997b4 in networkReloadFirewallRulesHelper (obj=0x7fffd4028250, opaque=0x0) at ../src/network/bridge_driver.c:1703
  #2  0x00007ffff7a6b09b in virNetworkObjListForEachHelper (payload=0x7fffd4028250, ...) at ../src/conf/virnetworkobj.c:1414
  #3  0x00007ffff79287b6 in virHashForEachSafe (...) at ../src/util/virhash.c:387
  #4  0x00007ffff7a6b119 in virNetworkObjListForEach (...) at ../src/conf/virnetworkobj.c:1441
  #5  0x00007ffff7f99978 in networkReloadFirewallRules (...) at ../src/network/bridge_driver.c:1742
  #6  0x00007ffff7f962f2 in networkStateInitialize (...) at ../src/network/bridge_driver.c:645

Since virNetworkObjSetFwRemoval() does not free the object stored
in the first call, the second call just overwrites the stored
pointer leading to a memory leak:

  5,530 (48 direct, 5,482 indirect) bytes in 1 blocks are definitely lost in loss record 1,863 of 1,880
     at 0x4848C43: calloc (vg_replace_malloc.c:1595)
     by 0x4F1E979: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.7800.6)
     by 0x4976E32: virFirewallNew (virfirewall.c:118)
     by 0x4979BA9: virFirewallParseXML (virfirewall.c:1071)
     by 0x4ABEB1E: virNetworkLoadState (virnetworkobj.c:938)
     by 0x4ABF35C: virNetworkObjLoadAllState (virnetworkobj.c:1072)
     by 0x4E9A25E: networkStateInitialize (bridge_driver.c:624)
     by 0x4CB1FA6: virStateInitialize (libvirt.c:665)
     by 0x15A6C6: daemonRunStateInit (remote_daemon.c:611)
     by 0x49E69F0: virThreadHelper (virthread.c:256)
     by 0x532B428: start_thread (in /lib64/libc.so.6)
     by 0x5397373: clone (in /lib64/libc.so.6)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/virnetworkobj.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index 19305798cb..5abc295506 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -255,6 +255,7 @@ void
 virNetworkObjSetFwRemoval(virNetworkObj *obj,
                           virFirewall *fwRemoval)
 {
+    virFirewallFree(obj->fwRemoval);
     obj->fwRemoval = fwRemoval;
     /* give it a name so it's identifiable in the XML */
     if (fwRemoval)
-- 
2.44.2

On Fri, Jun 14, 2024 at 03:07:00PM +0200, Michal Privoznik wrote:
> The virNetworkObjSetFwRemoval() function is called at least two
> times when there's a network running and network driver
> initializes:
> 
> 1) when loading state XMLs:
>   #0  virNetworkObjSetFwRemoval (obj=0x7fffd4028250, fwRemoval=0x7fffd4020ad0) at ../src/conf/virnetworkobj.c:258
>   #1  0x00007ffff7a69c68 in virNetworkLoadState (...) at ../src/conf/virnetworkobj.c:952
>   #2  0x00007ffff7a6a35d in virNetworkObjLoadAllState (...) at ../src/conf/virnetworkobj.c:1072
>   #3  0x00007ffff7f9625f in networkStateInitialize (...) at ../src/network/bridge_driver.c:624
> 
> 2) when firewall rules are being reloaded:
>   #0  virNetworkObjSetFwRemoval (obj=0x7fffd4028250, fwRemoval=0x7fffd402e5b0) at ../src/conf/virnetworkobj.c:258
>   #1  0x00007ffff7f997b4 in networkReloadFirewallRulesHelper (obj=0x7fffd4028250, opaque=0x0) at ../src/network/bridge_driver.c:1703
>   #2  0x00007ffff7a6b09b in virNetworkObjListForEachHelper (payload=0x7fffd4028250, ...) at ../src/conf/virnetworkobj.c:1414
>   #3  0x00007ffff79287b6 in virHashForEachSafe (...) at ../src/util/virhash.c:387
>   #4  0x00007ffff7a6b119 in virNetworkObjListForEach (...) at ../src/conf/virnetworkobj.c:1441
>   #5  0x00007ffff7f99978 in networkReloadFirewallRules (...) at ../src/network/bridge_driver.c:1742
>   #6  0x00007ffff7f962f2 in networkStateInitialize (...) at ../src/network/bridge_driver.c:645
> 
> Since virNetworkObjSetFwRemoval() does not free the object stored
> in the first call, the second call just overwrites the stored
> pointer leading to a memory leak:
> 
>   5,530 (48 direct, 5,482 indirect) bytes in 1 blocks are definitely lost in loss record 1,863 of 1,880
>      at 0x4848C43: calloc (vg_replace_malloc.c:1595)
>      by 0x4F1E979: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.7800.6)
>      by 0x4976E32: virFirewallNew (virfirewall.c:118)
>      by 0x4979BA9: virFirewallParseXML (virfirewall.c:1071)
>      by 0x4ABEB1E: virNetworkLoadState (virnetworkobj.c:938)
>      by 0x4ABF35C: virNetworkObjLoadAllState (virnetworkobj.c:1072)
>      by 0x4E9A25E: networkStateInitialize (bridge_driver.c:624)
>      by 0x4CB1FA6: virStateInitialize (libvirt.c:665)
>      by 0x15A6C6: daemonRunStateInit (remote_daemon.c:611)
>      by 0x49E69F0: virThreadHelper (virthread.c:256)
>      by 0x532B428: start_thread (in /lib64/libc.so.6)
>      by 0x5397373: clone (in /lib64/libc.so.6)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/conf/virnetworkobj.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrangé <berrange@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 :|