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@xxxxxxxxxx>
> CC: Peter Krempa <pkrempa@xxxxxxxxxx>
> CC: Roman Grigoriev <rgrigoriev@xxxxxxxxxxxxx>
> ---
> 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.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
