Re: [PATCH 1/2] remote: cleanup properly virDomainDef in ACL helpers

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(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); 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.