[libvirt] [PATCH 0/2] Fix two valgrind problems
Ideally, these would go into the release (at least 1/2 as it's a double free). The second one is just a leak so that one can wait if needed. Michal Privoznik (2): virCgroupValidateMachineGroup: Don't free @machinename virNetDaemonCallInhibit: Call virNetDaemonGotInhibitReply properly src/rpc/virnetdaemon.c | 17 +++++++++++------ src/util/vircgroup.c | 3 +-- 2 files changed, 12 insertions(+), 8 deletions(-) -- 2.13.0
We are given a string in @machinename, we never allocate it, just merely use it for reading. We should not free it otherwise it leads to double free: ==32191== Thread 17: ==32191== Invalid free() / delete / delete[] / realloc() ==32191== at 0x4C2D1A0: free (vg_replace_malloc.c:530) ==32191== by 0x54BBB84: virFree (viralloc.c:582) ==32191== by 0x2BC04499: qemuProcessStop (qemu_process.c:6313) ==32191== by 0x2BC500FF: processMonitorEOFEvent (qemu_driver.c:4724) ==32191== by 0x2BC502FC: qemuProcessEventHandler (qemu_driver.c:4769) ==32191== by 0x5550640: virThreadPoolWorker (virthreadpool.c:167) ==32191== by 0x554FBCF: virThreadHelper (virthread.c:206) ==32191== by 0x8F913D3: start_thread (in /lib64/libpthread-2.23.so) ==32191== by 0x928DE3C: clone (in /lib64/libc-2.23.so) ==32191== Address 0x31893d70 is 0 bytes inside a block of size 1,100 free'd ==32191== at 0x4C2D1A0: free (vg_replace_malloc.c:530) ==32191== by 0x54BBB84: virFree (viralloc.c:582) ==32191== by 0x54C1936: virCgroupValidateMachineGroup (vircgroup.c:343) ==32191== by 0x54C4B29: virCgroupNewDetectMachine (vircgroup.c:1550) ==32191== by 0x2BBDDA29: qemuConnectCgroup (qemu_cgroup.c:972) ==32191== by 0x2BC05DA7: qemuProcessReconnect (qemu_process.c:6822) ==32191== by 0x554FBCF: virThreadHelper (virthread.c:206) ==32191== by 0x8F913D3: start_thread (in /lib64/libpthread-2.23.so) ==32191== by 0x928DE3C: clone (in /lib64/libc-2.23.so) ==32191== Block was alloc'd at ==32191== at 0x4C2BE80: malloc (vg_replace_malloc.c:298) ==32191== by 0x4C2E35F: realloc (vg_replace_malloc.c:785) ==32191== by 0x54BB492: virReallocN (viralloc.c:245) ==32191== by 0x54BEDF2: virBufferGrow (virbuffer.c:150) ==32191== by 0x54BF3B9: virBufferVasprintf (virbuffer.c:408) ==32191== by 0x54BF324: virBufferAsprintf (virbuffer.c:381) ==32191== by 0x55BB271: virDomainGenerateMachineName (domain_conf.c:27078) ==32191== by 0x2BBD5B8F: qemuDomainGetMachineName (qemu_domain.c:9595) ==32191== by 0x2BBDD9B4: qemuConnectCgroup (qemu_cgroup.c:966) ==32191== by 0x2BC05DA7: qemuProcessReconnect (qemu_process.c:6822) ==32191== by 0x554FBCF: virThreadHelper (virthread.c:206) ==32191== by 0x8F913D3: start_thread (in /lib64/libpthread-2.23.so) Moreover, make the @machinename 'const char *' to mark it explicitly that we are not changing the passed string. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircgroup.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index f274aee81..0a31947b0 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -253,7 +253,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, const char *name, const char *drivername, bool stripEmulatorSuffix, - char *machinename) + const char *machinename) { size_t i; bool valid = false; @@ -340,7 +340,6 @@ virCgroupValidateMachineGroup(virCgroupPtr group, VIR_FREE(partname); VIR_FREE(scopename_old); VIR_FREE(scopename_new); - VIR_FREE(machinename); return valid; } -- 2.13.0
So there are couple of issues here. Firstly, we never unref the @pendingReply and thus it leaks. ==13279== 144 (72 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 1,095 of 1,259 ==13279== at 0x4C2E080: calloc (vg_replace_malloc.c:711) ==13279== by 0x781FA97: _dbus_pending_call_new_unlocked (in /usr/lib64/libdbus-1.so.3.14.11) ==13279== by 0x7812A4C: dbus_connection_send_with_reply (in /usr/lib64/libdbus-1.so.3.14.11) ==13279== by 0x56BEDF3: virNetDaemonCallInhibit (virnetdaemon.c:514) ==13279== by 0x56BEF18: virNetDaemonAddShutdownInhibition (virnetdaemon.c:536) ==13279== by 0x12473B: daemonInhibitCallback (libvirtd.c:742) ==13279== by 0x1249BD: daemonRunStateInit (libvirtd.c:823) ==13279== by 0x554FBCF: virThreadHelper (virthread.c:206) ==13279== by 0x8F913D3: start_thread (in /lib64/libpthread-2.23.so) ==13279== by 0x928DE3C: clone (in /lib64/libc-2.23.so) Secondly, while we send the message, we are suspended ('cos we're talking to a UNIX socket). However, until we are resumed back again the reply might have came therefore subsequent dbus_pending_call_set_notify() has no effect and in fact the virNetDaemonGotInhibitReply() callback is never called. Thirdly, the dbus_connection_send_with_reply() has really stupid policy for return values. To cite the man page: Returns FALSE if no memory, TRUE otherwise. Yes, that's right. If anything goes wrong and it's not case of OOM then TRUE is returned, i.e. you're trying to pass FDs and it's not supported, or you're not connected, or anything else. Therefore, checking for return value of dbus_connection_send_with_reply() is not enoguh. We also have to check if @pendingReply is not NULL before proceeding any further. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnetdaemon.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index e805e3a3c..00247cfc3 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -471,6 +471,7 @@ virNetDaemonGotInhibitReply(DBusPendingCall *pending, cleanup: virObjectUnlock(dmn); + dbus_pending_call_unref(pending); } @@ -483,7 +484,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn, const char *mode) { DBusMessage *message; - DBusPendingCall *pendingReply; + DBusPendingCall *pendingReply = NULL; DBusConnection *systemBus; VIR_DEBUG("dmn=%p what=%s who=%s why=%s mode=%s", @@ -510,13 +511,17 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn, DBUS_TYPE_STRING, &mode, DBUS_TYPE_INVALID); - pendingReply = NULL; if (dbus_connection_send_with_reply(systemBus, message, &pendingReply, - 25*1000)) { - dbus_pending_call_set_notify(pendingReply, - virNetDaemonGotInhibitReply, - dmn, NULL); + 25 * 1000) && + pendingReply) { + if (dbus_pending_call_get_completed(pendingReply)) { + virNetDaemonGotInhibitReply(pendingReply, dmn); + } else { + dbus_pending_call_set_notify(pendingReply, + virNetDaemonGotInhibitReply, + dmn, NULL); + } dmn->autoShutdownCallingInhibit = true; } virDBusMessageUnref(message); -- 2.13.0
On Tue, Aug 01, 2017 at 11:41:24AM +0200, Michal Privoznik wrote:
Ideally, these would go into the release (at least 1/2 as it's a double free). The second one is just a leak so that one can wait if needed.
Michal Privoznik (2): virCgroupValidateMachineGroup: Don't free @machinename virNetDaemonCallInhibit: Call virNetDaemonGotInhibitReply properly
ACK series Jan
participants (2)
-
Ján Tomko -
Michal Privoznik