9:06 a.m.
The third patch MIGHT fix the following issue:
https://issues.redhat.com/browse/RHEL-22574
but at this point it's still unclear. I'll append appropriate
'Resolves:' line when I learn more.
Michal Prívozník (3):
virfirewall: Fir a memleak in virFirewallParseXML()
virnetworkobj: Free fwRemoval before setting another one in
virNetworkObjSetFwRemoval()
remote_daemon_dispatch: Unref sasl session when closing client
connection
src/conf/virnetworkobj.c | 1 +
src/remote/remote_daemon_dispatch.c | 4 ++++
src/util/virfirewall.c | 2 +-
3 files changed, 6 insertions(+), 1 deletion(-)
--
2.44.2
9:06 a.m.
New subject: [PATCH 1/3] virfirewall: Fir a memleak in virFirewallParseXML()
As a part of parsing XML, virFirewallParseXML() calls
virXMLNodeContentString() and then passes the return value
further. But virXMLNodeContentString() is documented so that it's
the caller's responsibility to free the returned string, which
virFirewallParseXML() never does. This leads to a memory leak:
14,300 bytes in 220 blocks are definitely lost in loss record 1,879 of 1,891
at 0x4841858: malloc (vg_replace_malloc.c:442)
by 0x5491E3C: xmlBufCreateSize (in /usr/lib64/libxml2.so.2.12.6)
by 0x54C2401: xmlNodeGetContent (in /usr/lib64/libxml2.so.2.12.6)
by 0x49F7791: virXMLNodeContentString (virxml.c:354)
by 0x4979F25: virFirewallParseXML (virfirewall.c:1134)
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)
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virfirewall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 2219506b18..ae81ca52c6 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -1131,7 +1131,7 @@ virFirewallParseXML(virFirewall **firewall,
NULL, NULL, NULL);
for (i = 0; i < nargs; i++) {
- char *arg = virXMLNodeContentString(argsNodes[i]);
+ g_autofree char *arg = virXMLNodeContentString(argsNodes[i]);
if (!arg)
return -1;
--
2.44.2
9:18 a.m.
New subject: [PATCH 1/3] virfirewall: Fir a memleak in virFirewallParseXML()
On Fri, Jun 14, 2024 at 03:06:59PM +0200, Michal Privoznik wrote:
As a part of parsing XML, virFirewallParseXML() calls
virXMLNodeContentString() and then passes the return value
further. But virXMLNodeContentString() is documented so that it's
the caller's responsibility to free the returned string, which
virFirewallParseXML() never does. This leads to a memory leak:
14,300 bytes in 220 blocks are definitely lost in loss record 1,879 of 1,891
at 0x4841858: malloc (vg_replace_malloc.c:442)
by 0x5491E3C: xmlBufCreateSize (in /usr/lib64/libxml2.so.2.12.6)
by 0x54C2401: xmlNodeGetContent (in /usr/lib64/libxml2.so.2.12.6)
by 0x49F7791: virXMLNodeContentString (virxml.c:354)
by 0x4979F25: virFirewallParseXML (virfirewall.c:1134)
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)
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virfirewall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
9:07 a.m.
New subject: [PATCH 2/3] virnetworkobj: Free fwRemoval before setting another one in virNetworkObjSetFwRemoval()
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(a)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
9:18 a.m.
New subject: [PATCH 2/3] virnetworkobj: Free fwRemoval before setting another
one in virNetworkObjSetFwRemoval()
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(a)redhat.com>
---
src/conf/virnetworkobj.c | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
9:07 a.m.
New subject: [PATCH 3/3] remote_daemon_dispatch: Unref sasl session when closing client connection
In ideal world, where clients close connection gracefully their
SASL session is freed in virNetServerClientDispose() as it's
stored in client->sasl. Unfortunately, if client connection is
closed prematurely (e.g. the moment virsh asks for credentials),
the _virNetServerClient member is never set and corresponding
SASL session is never freed. The handler is still stored in
client private data, so free it in remoteClientCloseFunc().
20,862 (288 direct, 20,574 indirect) bytes in 3 blocks are definitely lost in loss
record 1,763 of 1,772
at 0x50390C4: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501BDAF: g_object_new_internal.part.0 (in
/usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501D43D: g_object_new_with_properties (in
/usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501E318: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x49BAA63: virObjectNew (virobject.c:252)
by 0x49BABC6: virObjectLockableNew (virobject.c:274)
by 0x4B0526C: virNetSASLSessionNewServer (virnetsaslcontext.c:230)
by 0x18EEFC: remoteDispatchAuthSaslInit (remote_daemon_dispatch.c:3696)
by 0x15E128: remoteDispatchAuthSaslInitHelper (remote_daemon_dispatch_stubs.h:74)
by 0x4B0FA5E: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
by 0x4B0F591: virNetServerProgramDispatch (virnetserverprogram.c:299)
by 0x4B18AE3: virNetServerProcessMsg (virnetserver.c:135)
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/remote/remote_daemon_dispatch.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index cfc1067e40..c88c9f5b15 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1776,6 +1776,10 @@ static void remoteClientCloseFunc(virNetServerClient *client)
daemonRemoveAllClientStreams(priv->streams);
remoteClientFreePrivateCallbacks(priv);
+
+#if WITH_SASL
+ virObjectUnref(priv->sasl);
+#endif
}
--
2.44.2
9:17 a.m.
New subject: [PATCH 3/3] remote_daemon_dispatch: Unref sasl session when
closing client connection
On Fri, Jun 14, 2024 at 03:07:01PM +0200, Michal Privoznik wrote:
In ideal world, where clients close connection gracefully their
SASL session is freed in virNetServerClientDispose() as it's
stored in client->sasl. Unfortunately, if client connection is
closed prematurely (e.g. the moment virsh asks for credentials),
the _virNetServerClient member is never set and corresponding
SASL session is never freed. The handler is still stored in
client private data, so free it in remoteClientCloseFunc().
20,862 (288 direct, 20,574 indirect) bytes in 3 blocks are definitely lost in loss
record 1,763 of 1,772
at 0x50390C4: g_type_create_instance (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501BDAF: g_object_new_internal.part.0 (in
/usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501D43D: g_object_new_with_properties (in
/usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x501E318: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7800.6)
by 0x49BAA63: virObjectNew (virobject.c:252)
by 0x49BABC6: virObjectLockableNew (virobject.c:274)
by 0x4B0526C: virNetSASLSessionNewServer (virnetsaslcontext.c:230)
by 0x18EEFC: remoteDispatchAuthSaslInit (remote_daemon_dispatch.c:3696)
by 0x15E128: remoteDispatchAuthSaslInitHelper (remote_daemon_dispatch_stubs.h:74)
by 0x4B0FA5E: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
by 0x4B0F591: virNetServerProgramDispatch (virnetserverprogram.c:299)
by 0x4B18AE3: virNetServerProcessMsg (virnetserver.c:135)
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/remote/remote_daemon_dispatch.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index cfc1067e40..c88c9f5b15 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1776,6 +1776,10 @@ static void remoteClientCloseFunc(virNetServerClient *client)
daemonRemoveAllClientStreams(priv->streams);
remoteClientFreePrivateCallbacks(priv);
+
+#if WITH_SASL
+ virObjectUnref(priv->sasl);
+#endif
}
Since the virNetServerClient may live on for a bit after CloseFunc is
called, I'd be happier with
g_clear_pointer(&priv->sasl, virObjectUnref);
to prevent an accidental use after free later. With that changed
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
355
days inactive
355
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik