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@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.
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.
That is all beyond my motivation in this patch.
Den
0000000000026716 <remoteRelayDomainEventCheckACL>:
26716: f3 0f 1e fa endbr64
2671a: 55 push %rbp
2671b: 48 89 e5 mov %rsp,%rbp
2671e: 41 56 push %r14
26720: 41 55 push %r13
26722: 41 54 push %r12
26724: 53 push %rbx
26725: 48 83 ec 40 sub $0x40,%rsp
26729: 48 89 7d b8 mov %rdi,-0x48(%rbp)
2672d: 48 89 75 b0 mov %rsi,-0x50(%rbp)
26731: 48 89 55 a8 mov %rdx,-0x58(%rbp)
26735: 64 48 8b 04 25 28 00 mov %fs:0x28,%rax
2673c: 00 00
2673e: 48 89 45 d8 mov %rax,-0x28(%rbp)
26742: 31 c0 xor %eax,%eax
26744: be e8 06 00 00 mov $0x6e8,%esi
26749: bf 01 00 00 00 mov $0x1,%edi
2674e: e8 00 00 00 00 call 26753 <remoteRelayDomainEventCheckACL+0x3d>
2674f: R_X86_64_PLT32 g_malloc0_n-0x4
26753: 48 89 45 c8 mov %rax,-0x38(%rbp)
26757: 48 c7 45 d0 00 00 00 movq $0x0,-0x30(%rbp)
2675e: 00
2675f: c6 45 c3 00 movb $0x0,-0x3d(%rbp)
26763: 48 8b 45 c8 mov -0x38(%rbp),%rax
26767: 48 8b 55 a8 mov -0x58(%rbp),%rdx
2676b: 48 8b 52 20 mov 0x20(%rdx),%rdx
2676f: 48 89 50 30 mov %rdx,0x30(%rax)
26773: 48 8b 45 a8 mov -0x58(%rbp),%rax
26777: 48 83 c0 2c add $0x2c,%rax
2677b: 48 8b 55 c8 mov -0x38(%rbp),%rdx
2677f: 48 8d 4a 08 lea 0x8(%rdx),%rcx
26783: 48 8b 50 08 mov 0x8(%rax),%rdx
26787: 48 8b 00 mov (%rax),%rax
2678a: 48 89 01 mov %rax,(%rcx)
2678d: 48 89 51 08 mov %rdx,0x8(%rcx)
26791: 48 8b 45 b8 mov -0x48(%rbp),%rax
26795: 48 89 c7 mov %rax,%rdi
26798: e8 00 00 00 00 call 2679d <remoteRelayDomainEventCheckACL+0x87>
26799: R_X86_64_PLT32 virNetServerClientGetIdentity-0x4
2679d: 48 89 45 d0 mov %rax,-0x30(%rbp)
267a1: 48 8b 45 d0 mov -0x30(%rbp),%rax
267a5: 48 85 c0 test %rax,%rax
267a8: 74 28 je 267d2 <remoteRelayDomainEventCheckACL+0xbc>
267aa: 48 8b 45 d0 mov -0x30(%rbp),%rax
267ae: 48 89 c7 mov %rax,%rdi
267b1: e8 00 00 00 00 call 267b6 <remoteRelayDomainEventCheckACL+0xa0>
267b2: R_X86_64_PLT32 virIdentitySetCurrent-0x4
267b6: 85 c0 test %eax,%eax
267b8: 78 1b js 267d5 <remoteRelayDomainEventCheckACL+0xbf>
267ba: 48 8b 55 c8 mov -0x38(%rbp),%rdx
267be: 48 8b 45 b0 mov -0x50(%rbp),%rax
267c2: 48 89 d6 mov %rdx,%rsi
267c5: 48 89 c7 mov %rax,%rdi
267c8: e8 00 00 00 00 call 267cd <remoteRelayDomainEventCheckACL+0xb7>
267c9: R_X86_64_PLT32 virConnectDomainEventRegisterAnyCheckACL-0x4
267cd: 88 45 c3 mov %al,-0x3d(%rbp)
267d0: eb 04 jmp 267d6 <remoteRelayDomainEventCheckACL+0xc0>
267d2: 90 nop
267d3: eb 01 jmp 267d6 <remoteRelayDomainEventCheckACL+0xc0>
267d5: 90 nop
267d6: bf 00 00 00 00 mov $0x0,%edi
267db: e8 00 00 00 00 call 267e0 <remoteRelayDomainEventCheckACL+0xca>
267dc: R_X86_64_PLT32 virIdentitySetCurrent-0x4
267e0: 89 45 c4 mov %eax,-0x3c(%rbp)
267e3: 44 0f b6 6d c3 movzbl -0x3d(%rbp),%r13d
267e8: 41 be 00 00 00 00 mov $0x0,%r14d
267ee: 48 8d 45 d0 lea -0x30(%rbp),%rax
267f2: 48 89 c7 mov %rax,%rdi
267f5: e8 01 99 fd ff call fb <glib_autoptr_cleanup_virIdentity>
267fa: 41 83 fe 01 cmp $0x1,%r14d
267fe: 74 3b je 2683b <remoteRelayDomainEventCheckACL+0x125>
26800: 41 bc 00 00 00 00 mov $0x0,%r12d
26806: 48 8d 45 c8 lea -0x38(%rbp),%rax
2680a: 48 89 c7 mov %rax,%rdi
2680d: e8 1a 98 fd ff call 2c <g_autoptr_cleanup_generic_gfree>
26812: 41 83 fc 01 cmp $0x1,%r12d
26816: 74 37 je 2684f <remoteRelayDomainEventCheckACL+0x139>
26818: 44 89 e8 mov %r13d,%eax
2681b: 48 8b 55 d8 mov -0x28(%rbp),%rdx
2681f: 64 48 2b 14 25 28 00 sub %fs:0x28,%rdx
26826: 00 00
26828: 74 49 je 26873 <remoteRelayDomainEventCheckACL+0x15d>
2682a: eb 42 jmp 2686e <remoteRelayDomainEventCheckACL+0x158>
2682c: f3 0f 1e fa endbr64
26830: 49 89 c4 mov %rax,%r12
26833: 41 be 01 00 00 00 mov $0x1,%r14d
26839: eb b3 jmp 267ee <remoteRelayDomainEventCheckACL+0xd8>
2683b: 4c 89 e3 mov %r12,%rbx
2683e: eb 07 jmp 26847 <remoteRelayDomainEventCheckACL+0x131>
26840: f3 0f 1e fa endbr64
26844: 48 89 c3 mov %rax,%rbx
26847: 41 bc 01 00 00 00 mov $0x1,%r12d
2684d: eb b7 jmp 26806 <remoteRelayDomainEventCheckACL+0xf0>
2684f: 48 89 d8 mov %rbx,%rax
26852: 48 8b 55 d8 mov -0x28(%rbp),%rdx
26856: 64 48 2b 14 25 28 00 sub %fs:0x28,%rdx
2685d: 00 00
2685f: 74 05 je 26866 <remoteRelayDomainEventCheckACL+0x150>
26861: e8 00 00 00 00 call 26866 <remoteRelayDomainEventCheckACL+0x150>
26862: R_X86_64_PLT32 __stack_chk_fail-0x4
26866: 48 89 c7 mov %rax,%rdi
26869: e8 00 00 00 00 call 2686e <remoteRelayDomainEventCheckACL+0x158>
2686a: R_X86_64_PLT32 _Unwind_Resume-0x4
2686e: e8 00 00 00 00 call 26873 <remoteRelayDomainEventCheckACL+0x15d>
2686f: R_X86_64_PLT32 __stack_chk_fail-0x4
26873: 48 83 c4 40 add $0x40,%rsp
26877: 5b pop %rbx
26878: 41 5c pop %r12
2687a: 41 5d pop %r13
2687c: 41 5e pop %r14
2687e: 5d pop %rbp
2687f: c3 ret
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
