[PATCH 0/5] GDBus and memleak fixes
The first patch fixes issue raised by Cole here: https://www.redhat.com/archives/libvir-list/2020-October/msg00037.html The second one sets the environment variable which will cause our tests fail should we run into similar problem. To see it in action just switch order with patch 1/5 and observe test suite failing. The last two patches fix memleaks I've found during debugging the original problem. https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/197284472 Michal Prívozník (5): lib: Don't unref message passed to virGDBusCallMethod{WithFD}() tests: Set G_DEBUG environment variable virfirewalltest: Don't duplicate string when adding it onto stringlist vmx; Free @checkMACAddress in virVMXParseEthernet() NEWS: Document recent GLib DBus fix NEWS.rst | 6 ++++++ src/rpc/virnetdaemon.c | 4 ++-- src/util/virfirewalld.c | 16 ++++++++-------- src/util/virgdbus.c | 29 ++++++++++++++++++++--------- src/util/virgdbus.h | 4 ++-- src/util/virpolkit.c | 4 ++-- src/util/virsystemd.c | 23 ++++++++--------------- src/vmx/vmx.c | 1 + tests/meson.build | 1 + tests/virfirewalltest.c | 2 +- 10 files changed, 51 insertions(+), 39 deletions(-) -- 2.26.2
The virGDBusCallMethod() and virGDBusCallMethodWithFD() are simple wrappers over g_dbus_connection_call_sync() and g_dbus_connection_call_with_unix_fd_list_sync() respectively. The documentation to these function states that passed parameters (@message in our case) is consumed for 'convenient' inline use of g_variant_new() [1]. But that means we must not unref the message afterwards. To make it explicit that the message is consumed the signature of our wrappers is changed too. 1: https://developer.gnome.org/gio/stable/GDBusConnection.html#g-dbus-connectio... Reported-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetdaemon.c | 4 ++-- src/util/virfirewalld.c | 16 ++++++++-------- src/util/virgdbus.c | 29 ++++++++++++++++++++--------- src/util/virgdbus.h | 4 ++-- src/util/virpolkit.c | 4 ++-- src/util/virsystemd.c | 23 ++++++++--------------- 6 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 37a5662e04..b42feb7f1e 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -469,7 +469,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn, { g_autoptr(GVariant) reply = NULL; g_autoptr(GUnixFDList) replyFD = NULL; - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; GDBusConnection *systemBus; int fd; int rc; @@ -494,7 +494,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn, "/org/freedesktop/login1", "org.freedesktop.login1.Manager", "Inhibit", - message, + &message, NULL); if (rc < 0) diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c index a94ac7c183..b00f3bf418 100644 --- a/src/util/virfirewalld.c +++ b/src/util/virfirewalld.c @@ -83,7 +83,7 @@ int virFirewallDGetVersion(unsigned long *version) { GDBusConnection *sysbus = virGDBusGetSystemBus(); - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; g_autoptr(GVariant) reply = NULL; g_autoptr(GVariant) gvar = NULL; char *versionStr; @@ -101,7 +101,7 @@ virFirewallDGetVersion(unsigned long *version) "/org/fedoraproject/FirewallD1", "org.freedesktop.DBus.Properties", "Get", - message) < 0) + &message) < 0) return -1; g_variant_get(reply, "(v)", &gvar); @@ -129,7 +129,7 @@ int virFirewallDGetBackend(void) { GDBusConnection *sysbus = virGDBusGetSystemBus(); - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; g_autoptr(GVariant) reply = NULL; g_autoptr(GVariant) gvar = NULL; g_autoptr(virError) error = NULL; @@ -154,7 +154,7 @@ virFirewallDGetBackend(void) "/org/fedoraproject/FirewallD1/config", "org.freedesktop.DBus.Properties", "Get", - message) < 0) + &message) < 0) return -1; if (error->level == VIR_ERR_ERROR) { @@ -273,7 +273,7 @@ virFirewallDApplyRule(virFirewallLayer layer, { const char *ipv = virFirewallLayerFirewallDTypeToString(layer); GDBusConnection *sysbus = virGDBusGetSystemBus(); - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; g_autoptr(GVariant) reply = NULL; g_autoptr(virError) error = NULL; @@ -304,7 +304,7 @@ virFirewallDApplyRule(virFirewallLayer layer, "/org/fedoraproject/FirewallD1", "org.fedoraproject.FirewallD1.direct", "passthrough", - message) < 0) + &message) < 0) return -1; if (error->level == VIR_ERR_ERROR) { @@ -353,7 +353,7 @@ virFirewallDInterfaceSetZone(const char *iface, const char *zone) { GDBusConnection *sysbus = virGDBusGetSystemBus(); - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; if (!sysbus) return -1; @@ -368,5 +368,5 @@ virFirewallDInterfaceSetZone(const char *iface, "/org/fedoraproject/FirewallD1", "org.fedoraproject.FirewallD1.zone", "changeZoneOfInterface", - message); + &message); } diff --git a/src/util/virgdbus.c b/src/util/virgdbus.c index 4360a6acff..856ad80a88 100644 --- a/src/util/virgdbus.c +++ b/src/util/virgdbus.c @@ -205,23 +205,26 @@ virGDBusCallMethod(GDBusConnection *conn, const char *objectPath, const char *ifaceName, const char *method, - GVariant *data) + GVariant **data) { g_autoptr(GVariant) ret = NULL; g_autoptr(GError) gerror = NULL; + GVariant *parameters = NULL; if (error) memset(error, 0, sizeof(*error)); - if (data) - g_variant_ref_sink(data); + if (data && *data) { + parameters = g_variant_ref_sink(*data); + *data = NULL; + } ret = g_dbus_connection_call_sync(conn, busName, objectPath, ifaceName, method, - data, + parameters, replyType, G_DBUS_CALL_FLAGS_NONE, VIR_DBUS_METHOD_CALL_TIMEOUT_MILIS, @@ -259,24 +262,27 @@ virGDBusCallMethodWithFD(GDBusConnection *conn, const char *objectPath, const char *ifaceName, const char *method, - GVariant *data, + GVariant **data, GUnixFDList *dataFD) { g_autoptr(GVariant) ret = NULL; g_autoptr(GError) gerror = NULL; + GVariant *parameters = NULL; if (error) memset(error, 0, sizeof(*error)); - if (data) - g_variant_ref_sink(data); + if (data && *data) { + parameters = g_variant_ref_sink(*data); + *data = NULL; + } ret = g_dbus_connection_call_with_unix_fd_list_sync(conn, busName, objectPath, ifaceName, method, - data, + parameters, replyType, G_DBUS_CALL_FLAGS_NONE, VIR_DBUS_METHOD_CALL_TIMEOUT_MILIS, @@ -317,9 +323,14 @@ virGDBusCallMethodWithFD(GDBusConnection *conn G_GNUC_UNUSED, const char *objectPath G_GNUC_UNUSED, const char *ifaceName G_GNUC_UNUSED, const char *method G_GNUC_UNUSED, - GVariant *data G_GNUC_UNUSED, + GVariant **data, GUnixFDList *dataFD G_GNUC_UNUSED) { + if (data && *data) { + g_variant_unref(*data); + *data = NULL; + } + virReportSystemError(ENOSYS, "%s", _("Unix file descriptors not supported on this platform")); return -1; diff --git a/src/util/virgdbus.h b/src/util/virgdbus.h index ca7073e27c..9c3955073c 100644 --- a/src/util/virgdbus.h +++ b/src/util/virgdbus.h @@ -51,7 +51,7 @@ virGDBusCallMethod(GDBusConnection *conn, const char *objectPath, const char *ifaceName, const char *method, - GVariant *data); + GVariant **data); int virGDBusCallMethodWithFD(GDBusConnection *conn, @@ -63,7 +63,7 @@ virGDBusCallMethodWithFD(GDBusConnection *conn, const char *objectPath, const char *ifaceName, const char *method, - GVariant *data, + GVariant **data, GUnixFDList *dataFD); int diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index aad924a065..e9fb4ec64f 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -67,7 +67,7 @@ int virPolkitCheckAuth(const char *actionid, GVariantBuilder builder; GVariant *gprocess = NULL; GVariant *gdetails = NULL; - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; g_autoptr(GVariant) reply = NULL; g_autoptr(GVariantIter) iter = NULL; char *retkey; @@ -110,7 +110,7 @@ int virPolkitCheckAuth(const char *actionid, "/org/freedesktop/PolicyKit1/Authority", "org.freedesktop.PolicyKit1.Authority", "CheckAuthorization", - message) < 0) + &message) < 0) return -1; g_variant_get(reply, "((bba{ss}))", &is_authorized, &is_challenge, &iter); diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 8456085476..f849cbbf60 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -198,7 +198,7 @@ char * virSystemdGetMachineNameByPID(pid_t pid) { GDBusConnection *conn; - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; g_autoptr(GVariant) reply = NULL; g_autoptr(GVariant) gvar = NULL; g_autofree char *object = NULL; @@ -220,7 +220,7 @@ virSystemdGetMachineNameByPID(pid_t pid) "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", "GetMachineByPID", - message) < 0) + &message) < 0) return NULL; g_variant_get(reply, "(o)", &object); @@ -231,7 +231,6 @@ virSystemdGetMachineNameByPID(pid_t pid) VIR_DEBUG("Domain with pid %lld has object path '%s'", (long long) pid, object); - g_variant_unref(message); message = g_variant_new("(ss)", "org.freedesktop.machine1.Machine", "Name"); @@ -243,7 +242,7 @@ virSystemdGetMachineNameByPID(pid_t pid) object, "org.freedesktop.DBus.Properties", "Get", - message) < 0) + &message) < 0) return NULL; g_variant_get(reply, "(v)", &gvar); @@ -393,9 +392,7 @@ int virSystemdCreateMachine(const char *name, "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", "CreateMachineWithNetwork", - message); - - g_variant_unref(message); + &message); if (rc < 0) return -1; @@ -440,9 +437,7 @@ int virSystemdCreateMachine(const char *name, "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", "CreateMachine", - message); - - g_variant_unref(message); + &message); if (rc < 0) return -1; @@ -468,9 +463,7 @@ int virSystemdCreateMachine(const char *name, "/org/freedesktop/systemd1", "org.freedesktop.systemd1.Manager", "SetUnitProperties", - message); - - g_variant_unref(message); + &message); if (rc < 0) return -1; @@ -483,7 +476,7 @@ int virSystemdTerminateMachine(const char *name) { int rc; GDBusConnection *conn; - g_autoptr(GVariant) message = NULL; + GVariant *message = NULL; g_autoptr(virError) error = NULL; if (!name) @@ -519,7 +512,7 @@ int virSystemdTerminateMachine(const char *name) "/org/freedesktop/machine1", "org.freedesktop.machine1.Manager", "TerminateMachine", - message) < 0) + &message) < 0) return -1; if (error->level == VIR_ERR_ERROR && -- 2.26.2
On Fri, Oct 02, 2020 at 11:22:06AM +0200, Michal Privoznik wrote:
The virGDBusCallMethod() and virGDBusCallMethodWithFD() are simple wrappers over g_dbus_connection_call_sync() and g_dbus_connection_call_with_unix_fd_list_sync() respectively. The documentation to these function states that passed parameters (@message in our case) is consumed for 'convenient' inline use of g_variant_new() [1]. But that means we must not unref the message afterwards. To make it explicit that the message is consumed the signature of our wrappers is changed too.
1: https://developer.gnome.org/gio/stable/GDBusConnection.html#g-dbus-connectio...
Reported-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetdaemon.c | 4 ++-- src/util/virfirewalld.c | 16 ++++++++-------- src/util/virgdbus.c | 29 ++++++++++++++++++++--------- src/util/virgdbus.h | 4 ++-- src/util/virpolkit.c | 4 ++-- src/util/virsystemd.c | 23 ++++++++--------------- 6 files changed, 42 insertions(+), 38 deletions(-)
This doesn't look right, I know about the limitation and was counting with it when introducing virgdbus.c where we call g_variant_ref_sink() on the passed message. g_variant_new() returns floating reference which makes it possible to us it as described in the g_dbus_connection_call_sync() documentation but for our convenience I changed the behavior in virGDBusCallMethod() to make it a normal reference so it is not consumed. The motivation was to not introduce a new reference concept into libvirt code. I'll look into the issue but this should not happen. If you look into g_dbus_connection_call_sync() code in GLib they call g_variant_ref_sink() on the passed data as well and if they receive a normal reference they will simply add a new normal reference instead of consuming it. In addition the GLib g_dbus_connection_call_sync() should not be called from our tests because we mock this call. I'll look again into the issue to figure out what's wrong. For now NACK to this patch. Pavel
On 10/2/20 11:52 AM, Pavel Hrdina wrote:
On Fri, Oct 02, 2020 at 11:22:06AM +0200, Michal Privoznik wrote:
The virGDBusCallMethod() and virGDBusCallMethodWithFD() are simple wrappers over g_dbus_connection_call_sync() and g_dbus_connection_call_with_unix_fd_list_sync() respectively. The documentation to these function states that passed parameters (@message in our case) is consumed for 'convenient' inline use of g_variant_new() [1]. But that means we must not unref the message afterwards. To make it explicit that the message is consumed the signature of our wrappers is changed too.
1: https://developer.gnome.org/gio/stable/GDBusConnection.html#g-dbus-connectio...
Reported-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetdaemon.c | 4 ++-- src/util/virfirewalld.c | 16 ++++++++-------- src/util/virgdbus.c | 29 ++++++++++++++++++++--------- src/util/virgdbus.h | 4 ++-- src/util/virpolkit.c | 4 ++-- src/util/virsystemd.c | 23 ++++++++--------------- 6 files changed, 42 insertions(+), 38 deletions(-)
This doesn't look right, I know about the limitation and was counting with it when introducing virgdbus.c where we call g_variant_ref_sink() on the passed message.
g_variant_new() returns floating reference which makes it possible to us it as described in the g_dbus_connection_call_sync() documentation but for our convenience I changed the behavior in virGDBusCallMethod() to make it a normal reference so it is not consumed. The motivation was to not introduce a new reference concept into libvirt code.
I'll look into the issue but this should not happen. If you look into g_dbus_connection_call_sync() code in GLib they call g_variant_ref_sink() on the passed data as well and if they receive a normal reference they will simply add a new normal reference instead of consuming it.
In addition the GLib g_dbus_connection_call_sync() should not be called from our tests because we mock this call.
I'll look again into the issue to figure out what's wrong.
g_variant_ref_sink() does not do what you think. Here's the code: GVariant * g_variant_ref_sink (GVariant *value) { g_return_val_if_fail (value != NULL, NULL); g_return_val_if_fail (!g_atomic_ref_count_compare (&value->ref_count, 0), NULL); g_variant_lock (value); if (~value->state & STATE_FLOATING) g_variant_ref (value); else value->state &= ~STATE_FLOATING; g_variant_unlock (value); return value; } So if the variant has STATE_FLOATING bit set (which it does), it doesn't increase the refcounter. It just clears it. Michal
On Fri, Oct 02, 2020 at 12:04:21PM +0200, Michal Prívozník wrote:
On 10/2/20 11:52 AM, Pavel Hrdina wrote:
On Fri, Oct 02, 2020 at 11:22:06AM +0200, Michal Privoznik wrote:
The virGDBusCallMethod() and virGDBusCallMethodWithFD() are simple wrappers over g_dbus_connection_call_sync() and g_dbus_connection_call_with_unix_fd_list_sync() respectively. The documentation to these function states that passed parameters (@message in our case) is consumed for 'convenient' inline use of g_variant_new() [1]. But that means we must not unref the message afterwards. To make it explicit that the message is consumed the signature of our wrappers is changed too.
1: https://developer.gnome.org/gio/stable/GDBusConnection.html#g-dbus-connectio...
Reported-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetdaemon.c | 4 ++-- src/util/virfirewalld.c | 16 ++++++++-------- src/util/virgdbus.c | 29 ++++++++++++++++++++--------- src/util/virgdbus.h | 4 ++-- src/util/virpolkit.c | 4 ++-- src/util/virsystemd.c | 23 ++++++++--------------- 6 files changed, 42 insertions(+), 38 deletions(-)
This doesn't look right, I know about the limitation and was counting with it when introducing virgdbus.c where we call g_variant_ref_sink() on the passed message.
g_variant_new() returns floating reference which makes it possible to us it as described in the g_dbus_connection_call_sync() documentation but for our convenience I changed the behavior in virGDBusCallMethod() to make it a normal reference so it is not consumed. The motivation was to not introduce a new reference concept into libvirt code.
I'll look into the issue but this should not happen. If you look into g_dbus_connection_call_sync() code in GLib they call g_variant_ref_sink() on the passed data as well and if they receive a normal reference they will simply add a new normal reference instead of consuming it.
In addition the GLib g_dbus_connection_call_sync() should not be called from our tests because we mock this call.
I'll look again into the issue to figure out what's wrong.
g_variant_ref_sink() does not do what you think. Here's the code: GVariant * g_variant_ref_sink (GVariant *value) { g_return_val_if_fail (value != NULL, NULL); g_return_val_if_fail (!g_atomic_ref_count_compare (&value->ref_count, 0), NULL);
g_variant_lock (value);
if (~value->state & STATE_FLOATING) g_variant_ref (value); else value->state &= ~STATE_FLOATING;
g_variant_unlock (value);
return value; }
So if the variant has STATE_FLOATING bit set (which it does), it doesn't increase the refcounter. It just clears it.
I know how it works :) Our g_variant_ref_sink() call in virGDBusCallMethod() changes the data from floating reference to normal reference and the same call in g_dbus_connection_call_sync() adds another reference to the data so it looks like this: virGDBusCallMethod(); if (data) g_variant_ref_sink(); g_dbus_connection_call_sync(); if (data) message->body = g_variant_ref_sink (data); Now there are two normal references for data. Later g_dbus_connection_call_sync() frees the message which also removes the second reference from @data and the last reference should be freed by caller of virGDBusCallMethod(). The bug here is in the tests themselves, I'll post a patch in a minute. Pavel
With us switching to glib more and more it is easy to get things wrong (as can be seen in the previous commit). Set G_DEBUG variable to "fatal-warnings" which causes GLib to abort the program at the first call to g_warning() or g_critical(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/meson.build b/tests/meson.build index cb720f3afe..790033e243 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -36,6 +36,7 @@ tests_env = [ 'abs_top_srcdir=@0@'.format(meson.source_root()), 'LC_ALL=C', 'LIBVIRT_AUTOSTART=0', + 'G_DEBUG=fatal-warnings', ] if use_expensive_tests -- 2.26.2
On Fri, Oct 02, 2020 at 11:22:07AM +0200, Michal Privoznik wrote:
With us switching to glib more and more it is easy to get things wrong (as can be seen in the previous commit). Set G_DEBUG variable to "fatal-warnings" which causes GLib to abort the program at the first call to g_warning() or g_critical().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/meson.build | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
In our wrapper of g_dbus_connection_call_sync() in virfirewalltest a string is duplicated and added onto a virStringList. This leads to a memory leak because virStringListAdd() duplicates the string itself. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virfirewalltest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 607638e9d0..eed7bcbe50 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -119,7 +119,7 @@ VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, doError = true; } - virStringListAdd(&args, g_strdup(item)); + virStringListAdd(&args, item); } if (fwBuf) { -- 2.26.2
On Fri, Oct 02, 2020 at 11:22:08AM +0200, Michal Privoznik wrote:
In our wrapper of g_dbus_connection_call_sync() in virfirewalltest a string is duplicated and added onto a virStringList. This leads to a memory leak because virStringListAdd() duplicates the string itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virfirewalltest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The @checkMACAddress string is allocated in virVMXGetConfigString() but never freed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 4b1b04c6e1..9894d5c0ce 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -2734,6 +2734,7 @@ virVMXParseEthernet(virConfPtr conf, int controller, virDomainNetDefPtr *def) VIR_FREE(networkName); VIR_FREE(connectionType); VIR_FREE(addressType); + VIR_FREE(checkMACAddress); VIR_FREE(generatedAddress); VIR_FREE(address); VIR_FREE(virtualDev); -- 2.26.2
On Fri, Oct 02, 2020 at 11:22:09AM +0200, Michal Privoznik wrote:
The @checkMACAddress string is allocated in virVMXGetConfigString() but never freed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- NEWS.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 7663cf4208..6c237dbba8 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,12 @@ v6.9.0 (unreleased) * **Bug fixes** + * lib: Don't unref message passed to virGDBusCallMethod{WithFD}() + + The switch to GLib DBus implementation done in the previous release + contained a bug in which a DBus message was unrefed twice causing GLib to + abort the daemon. + v6.8.0 (2020-10-01) =================== -- 2.26.2
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Pavel Hrdina