Technically commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 does not really introduces a leak, but it is incorrect ideologically. Neither function accepting non-const pointer to virDomainDef does not provide any warrantee that the object will not be improved inside. Thus, keeping object model in mind, we must ensure that virDomainDefFree is called over virDomainDef object as a destructor. In order to achieve this we should change pointer declaration inside remoteRelayDomainEventCheckACL remoteRelayDomainQemuMonitorEventCheckACL and assign def->name via strdup. Fixes: 2ecdf259299813c2c674377e22a0acbce5ccbbb2 Signed-off-by: Denis V. Lunev <den(a)openvz.org&gt; CC: Peter Krempa <pkrempa(a)redhat.com&gt; CC: Roman Grigoriev <rgrigoriev(a)astralinux.ru&gt; --- src/remote/remote_daemon_dispatch.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index aaabd1e56c..3172a632df 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -154,14 +154,14 @@ static bool remoteRelayDomainEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) @@ -283,14 +283,14 @@ static bool remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClient *client, virConnectPtr conn, virDomainPtr dom) { - g_autofree virDomainDef *def = g_new0(virDomainDef, 1); + g_autoptr(virDomainDef) def = g_new0(virDomainDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def->name = dom->name; + def->name = g_strdup(dom->name); memcpy(def->uuid, dom->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) -- 2.40.1

On 3/18/24 09:37, Peter Krempa wrote: Nope. The code is written based on the manual code analysis. Sorry that commit message is not good enough. Please take a look into the attached disassembly of remoteRelayDomainEventCheckACL The key moment is that in the cleanup section we call g_autoptr_cleanup_generic_gfree to cleanup def pointer. That is not bad, but this is fragile. Right now this does not induce any leak, but once we will change something here (adding mode data) or plugins validating ACL will do something with the object (potentially they can) and we will have leaks with the dependent pointers. As far as I could talk about object model, if we call a constructor, we should call the destructor, namely glib_autoptr_cleanup_virDomainDef. That is all beyond my motivation in this patch. Den

Commit 2ecdf259299813c2c674377e22a0acbce5ccbbb2 is idealogically corrent, but unfortunately is incomplete. There are other similar objects in the module which are used also without proper initialization. This patch adds proper clauses to remoteRelayNetworkEventCheckACL remoteRelayStoragePoolEventCheckACL remoteRelayNodeDeviceEventCheckACL remoteRelaySecretEventCheckACL Signed-off-by: Denis V. Lunev <den(a)openvz.org&gt; CC: Peter Krempa <pkrempa(a)redhat.com&gt; CC: Roman Grigoriev <rgrigoriev(a)astralinux.ru&gt; --- src/remote/remote_daemon_dispatch.c | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 3172a632df..01f97bb345 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -180,21 +180,21 @@ static bool remoteRelayNetworkEventCheckACL(virNetServerClient *client, virConnectPtr conn, virNetworkPtr net) { - virNetworkDef def; + g_autoptr(virNetworkDef) def = g_new0(virNetworkDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virNetworkDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def.name = net->name; - memcpy(def.uuid, net->uuid, VIR_UUID_BUFLEN); + def->name = g_strdup(net->name); + memcpy(def->uuid, net->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) goto cleanup; if (virIdentitySetCurrent(identity) < 0) goto cleanup; - ret = virConnectNetworkEventRegisterAnyCheckACL(conn, &def); + ret = virConnectNetworkEventRegisterAnyCheckACL(conn, def); cleanup: ignore_value(virIdentitySetCurrent(NULL)); @@ -206,21 +206,21 @@ remoteRelayStoragePoolEventCheckACL(virNetServerClient *client, virConnectPtr conn, virStoragePoolPtr pool) { - virStoragePoolDef def; + g_autoptr(virStoragePoolDef) def = g_new0(virStoragePoolDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virStoragePoolDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def.name = pool->name; - memcpy(def.uuid, pool->uuid, VIR_UUID_BUFLEN); + def->name = g_strdup(pool->name); + memcpy(def->uuid, pool->uuid, VIR_UUID_BUFLEN); if (!(identity = virNetServerClientGetIdentity(client))) goto cleanup; if (virIdentitySetCurrent(identity) < 0) goto cleanup; - ret = virConnectStoragePoolEventRegisterAnyCheckACL(conn, &def); + ret = virConnectStoragePoolEventRegisterAnyCheckACL(conn, def); cleanup: ignore_value(virIdentitySetCurrent(NULL)); @@ -232,20 +232,20 @@ remoteRelayNodeDeviceEventCheckACL(virNetServerClient *client, virConnectPtr conn, virNodeDevicePtr dev) { - virNodeDeviceDef def; + g_autoptr(virNodeDeviceDef) def = g_new0(virNodeDeviceDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virNodeDeviceDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - def.name = dev->name; + def->name = g_strdup(dev->name); if (!(identity = virNetServerClientGetIdentity(client))) goto cleanup; if (virIdentitySetCurrent(identity) < 0) goto cleanup; - ret = virConnectNodeDeviceEventRegisterAnyCheckACL(conn, &def); + ret = virConnectNodeDeviceEventRegisterAnyCheckACL(conn, def); cleanup: ignore_value(virIdentitySetCurrent(NULL)); @@ -257,22 +257,22 @@ remoteRelaySecretEventCheckACL(virNetServerClient *client, virConnectPtr conn, virSecretPtr secret) { - virSecretDef def; + g_autoptr(virSecretDef) def = g_new0(virSecretDef, 1); g_autoptr(virIdentity) identity = NULL; bool ret = false; /* For now, we just create a virSecretDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ - memcpy(def.uuid, secret->uuid, VIR_UUID_BUFLEN); - def.usage_type = secret->usageType; - def.usage_id = secret->usageID; + memcpy(def->uuid, secret->uuid, VIR_UUID_BUFLEN); + def->usage_type = secret->usageType; + def->usage_id = secret->usageID; if (!(identity = virNetServerClientGetIdentity(client))) goto cleanup; if (virIdentitySetCurrent(identity) < 0) goto cleanup; - ret = virConnectSecretEventRegisterAnyCheckACL(conn, &def); + ret = virConnectSecretEventRegisterAnyCheckACL(conn, def); cleanup: ignore_value(virIdentitySetCurrent(NULL)); -- 2.40.1