On Sun, Mar 17, 2024 at 18:08:49 +0100, Denis V. Lunev wrote:
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.
I don't quite understand what the second sentence is supposed to mean, can you please elaborate the justification?
Thus, keeping object model in mind, we must ensure that virDomainDefFree is called over virDomainDef object as a destructor.
Using the object model as argument would require that you also use 'virDomainDefNew' as "constructor", which IMO in this case would be overkill same as using virDomainDefFree.
In order to achieve this we should change pointer declaration inside remoteRelayDomainEventCheckACL remoteRelayDomainQemuMonitorEventCheckACL and assign def->name via strdup.
Fixes: 2ecdf259299813c2c674377e22a0acbce5ccbbb2
The commit message doesn't really clarify what the actual problem is that this patch is fixing, which is needed in case you are mentioning that some commit is broken/being fixed.
Signed-off-by: Denis V. Lunev <den@openvz.org> CC: Peter Krempa <pkrempa@redhat.com> CC: Roman Grigoriev <rgrigoriev@astralinux.ru> --- 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);
Based on the CC-list I assume that there's a false positive from the static analysis tool which we've got multiple fixes/patches form recently. In such case I think it should be enough to explicitly clear def->name after use before freeing to show that we're intended to just borrow the value without going overkill on fully allocating and freeing the domain object. Alternatively a warning comment can be added too. Either way, please describe the reason for the patch in the commit message as requested above.