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

On 3/18/24 09:37, Peter Krempa wrote: > 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. Nope. The code is written based on the manual code analysis. Sorry that commit message is not good enough. > 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. > 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.