[PATCH] virtlogd: solve some memory leaks

Ján Tomko jtomko at redhat.com
Wed Jun 17 14:36:53 UTC 2020


On a Tuesday in 2020, wangjian wrote:
>We used asan to find some memory leaks in virtlogd. In the virThreadPoolFree function,
>When job->data is of type virNetServerJobPtr, the following memory leak problem exists.
>
>1. job->data is not released
>Direct leak of 24 byte(s) in 1 object(s) allocated from:
>  #0 0x7f14ab932560 in calloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560)  ??:?
>  #1 0x55ab07088853 in virAlloc (/usr/sbin/virtlogd+0x31853)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/viralloc.c:144
>  #2 0x55ab0707a515 (/usr/sbin/virtlogd+0x23515)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserver.c:209
>  #3 0x55ab07076d87 (/usr/sbin/virtlogd+0x1fd87)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverclient.c:1374
>  #4 0x55ab070770e2 (/usr/sbin/virtlogd+0x200e2)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverclient.c:1563
>  #5 0x55ab0709c67f in virEventPollRunOnce (/usr/sbin/virtlogd+0x4567f)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/vireventpoll.c:508
>  #6 0x55ab0709ad30 in virEventRunDefaultImpl (/usr/sbin/virtlogd+0x43d30)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/virevent.c:314
>  #7 0x55ab07079f6c in virNetDaemonRun (/usr/sbin/virtlogd+0x22f6c)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetdaemon.c:847
>  #8 0x55ab070714db in main (/usr/sbin/virtlogd+0x1a4db)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/logging/log_daemon.c:1162
>  #9 0x7f14aa3c2c56 in __libc_start_main (/usr/lib64/libc.so.6+0x25c56)  ??:?
>  #10 0x55ab07072639 in _start (/usr/sbin/virtlogd+0x1b639)  ??:?
>
>2. job->data->msg->buffer was not released
>Indirect leak of 152 byte(s) in 1 object(s) allocated from:
>  #0 0x7f14ab932750 in realloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7750)  ??:?
>  #1 0x55ab0708894d in virReallocN (/usr/sbin/virtlogd+0x3194d)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/viralloc.c:245
>  #2 0x55ab0707d830 in virNetMessageDecodeLength (/usr/sbin/virtlogd+0x26830)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetmessage.c:157
>  #3 0x55ab07076b36 (/usr/sbin/virtlogd+0x1fb36)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverclient.c:1269
>  #4 0x55ab070770e2 (/usr/sbin/virtlogd+0x200e2)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverclient.c:1563
>  #5 0x55ab0709c67f in virEventPollRunOnce (/usr/sbin/virtlogd+0x4567f)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/vireventpoll.c:508
>  #6 0x55ab0709ad30 in virEventRunDefaultImpl (/usr/sbin/virtlogd+0x43d30)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/virevent.c:314
>  #7 0x55ab07079f6c in virNetDaemonRun (/usr/sbin/virtlogd+0x22f6c)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetdaemon.c:847
>  #8 0x55ab070714db in main (/usr/sbin/virtlogd+0x1a4db)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/logging/log_daemon.c:1162
>  #9 0x7f14aa3c2c56 in __libc_start_main (/usr/lib64/libc.so.6+0x25c56)  ??:?
>  #10 0x55ab07072639 in _start (/usr/sbin/virtlogd+0x1b639)  ??:?
>
>3. job->data->msg was not released
>Indirect leak of 104 byte(s) in 1 object(s) allocated from:
>  #0 0x7f14ab932560 in calloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560)  ??:?
>  #1 0x55ab07088853 in virAlloc (/usr/sbin/virtlogd+0x31853)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/viralloc.c:144
>  #2 0x55ab0707d541 in virNetMessageNew (/usr/sbin/virtlogd+0x26541)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetmessage.c:42
>  #3 0x55ab07076618 (/usr/sbin/virtlogd+0x1f618)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverclient.c:416
>  #4 0x55ab0707755c in virNetServerClientNew (/usr/sbin/virtlogd+0x2055c)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverclient.c:467
>  #5 0x55ab0707a9df (/usr/sbin/virtlogd+0x239df)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserver.c:319
>  #6 0x55ab07075ad9 (/usr/sbin/virtlogd+0x1ead9)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverservice.c:88
>  #7 0x55ab0709c67f in virEventPollRunOnce (/usr/sbin/virtlogd+0x4567f)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/vireventpoll.c:508
>  #8 0x55ab0709ad30 in virEventRunDefaultImpl (/usr/sbin/virtlogd+0x43d30)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/virevent.c:314
>  #9 0x55ab07079f6c in virNetDaemonRun (/usr/sbin/virtlogd+0x22f6c)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetdaemon.c:847
>  #10 0x55ab070714db in main (/usr/sbin/virtlogd+0x1a4db)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/logging/log_daemon.c:1162
>  #11 0x7f14aa3c2c56 in __libc_start_main (/usr/lib64/libc.so.6+0x25c56)  ??:?
>  #12 0x55ab07072639 in _start (/usr/sbin/virtlogd+0x1b639)  ??:?
>
>4. job->data->prog was not released
>Indirect leak of 64 byte(s) in 1 object(s) allocated from:
>  #0 0x7f14ab932560 in calloc (/usr/local/gcc-6-4/lib64/libasan.so.3+0xc7560)  ??:?
>  #1 0x55ab07088d34 in virAllocVar (/usr/sbin/virtlogd+0x31d34)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/viralloc.c:560
>  #2 0x55ab070ac45c in virObjectNew (/usr/sbin/virtlogd+0x5545c)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/util/virobject.c:200
>  #3 0x55ab07074f06 in virNetServerProgramNew (/usr/sbin/virtlogd+0x1df06)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/rpc/virnetserverprogram.c:80
>  #4 0x55ab07071432 in main (/usr/sbin/virtlogd+0x1a432)  /usr/src/debug/libvirt-3.2.0-529.x86_64/src/logging/log_daemon.c:1131
>  #5 0x7f14aa3c2c56 in __libc_start_main (/usr/lib64/libc.so.6+0x25c56)  ??:?
>  #6 0x55ab07072639 in _start (/usr/sbin/virtlogd+0x1b639)  ??:?
>
>Signed-off-by: Jian wang <wangjian161 at huawei.com>
>---
> src/libvirt_private.syms |  1 +
> src/rpc/virnetserver.c   | 12 ++++++++++++
> src/util/virthreadpool.c |  9 +++++++++
> src/util/virthreadpool.h |  2 ++
> 4 files changed, 24 insertions(+)
>
>diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>index a6af44f..b23fc33 100644
>--- a/src/libvirt_private.syms
>+++ b/src/libvirt_private.syms
>@@ -3316,6 +3316,7 @@ virThreadPoolGetMaxWorkers;
> virThreadPoolGetMinWorkers;
> virThreadPoolGetPriorityWorkers;
> virThreadPoolNewFull;
>+virThreadPoolSetFreeFunc;
> virThreadPoolSendJob;
> virThreadPoolSetParameters;
>
>diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
>index e0a2386..66f0fb6 100644
>--- a/src/rpc/virnetserver.c
>+++ b/src/rpc/virnetserver.c
>@@ -188,6 +188,15 @@ virNetServerGetProgramLocked(virNetServerPtr srv,
>     return NULL;
> }
>
>+static void virNetServerHandleFree(void *jobOpaque)

virNetServerJobFree, no need for two verbs.

Also, using a free function in one place and open-coding it in HandleJob
is incosistent.

>+{
>+    virNetServerJobPtr job = jobOpaque;
>+
>+    virObjectUnref(job->prog);
>+    virNetMessageFree(job->msg);
>+    VIR_FREE(job);
>+}
>+
> static void
> virNetServerDispatchNewMessage(virNetServerClientPtr client,
>                                virNetMessagePtr msg,
>@@ -376,6 +385,9 @@ virNetServerPtr virNetServerNew(const char *name,
>                                               srv)))
>         goto error;
>
>+    if (srv->workers)
>+        virThreadPoolSetFreeFunc(srv->workers, virNetServerHandleFree);

If there is a need for a free function, it should be a parameter to
virThreadPoolNewFull, the other callers might need it too.

>+
>     srv->name = g_strdup(name);
>
>     srv->next_client_id = next_client_id;
>diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
>index 379d236..f885c04 100644
>--- a/src/util/virthreadpool.c
>+++ b/src/util/virthreadpool.c
>@@ -54,6 +54,7 @@ struct _virThreadPool {
>     bool quit;
>
>     virThreadPoolJobFunc jobFunc;
>+    virThreadPoolFreeFunc freeFunc;
>     const char *jobName;
>     void *jobOpaque;
>     virThreadPoolJobList jobList;
>@@ -271,6 +272,12 @@ virThreadPoolNewFull(size_t minWorkers,
>
> }
>
>+void virThreadPoolSetFreeFunc(virThreadPoolPtr pool,
>+                              virThreadPoolFreeFunc func)
>+{
>+    pool->freeFunc = func;
>+}
>+
> void virThreadPoolFree(virThreadPoolPtr pool)
> {
>     virThreadPoolJobPtr job;
>@@ -293,6 +300,8 @@ void virThreadPoolFree(virThreadPoolPtr pool)
>
>     while ((job = pool->jobList.head)) {
>         pool->jobList.head = pool->jobList.head->next;
>+        if (pool->freeFunc)
>+            pool->freeFunc(job->data);

I was not able to hit this code path in my testing - it seems
all the jobs get handled for me. A simple reproducer would really help.

And in virThreadPoolWorker we also call jobFunc and expect it to free
the job data. Calling jobFunc and freeFunc separately would look cleaner
to me.

Jano

>         VIR_FREE(job);
>     }
>
>diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h
>index c97d9b3..d2e24f7 100644
>--- a/src/util/virthreadpool.h
>+++ b/src/util/virthreadpool.h
>@@ -27,6 +27,7 @@ typedef struct _virThreadPool virThreadPool;
> typedef virThreadPool *virThreadPoolPtr;
>
> typedef void (*virThreadPoolJobFunc)(void *jobdata, void *opaque);
>+typedef void (*virThreadPoolFreeFunc)(void *jobdata);
>
> #define virThreadPoolNew(min, max, prio, func, opaque) \
>     virThreadPoolNewFull(min, max, prio, func, #func, opaque)
>@@ -46,6 +47,7 @@ size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool);
> size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool);
>
> void virThreadPoolFree(virThreadPoolPtr pool);
>+void virThreadPoolSetFreeFunc(virThreadPoolPtr pool, virThreadPoolFreeFunc func);
>
> int virThreadPoolSendJob(virThreadPoolPtr pool,
>                          unsigned int priority,
>-- 
>2.23.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200617/d4f3d883/attachment-0001.sig>


More information about the libvir-list mailing list