1. Patchew
  2. Libvirt
  3. View series

[PATCH] virtlogd: solve some memory leaks

wangjian posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ed0d4749-22f3-7fb9-034f-5f0e1ecfb76d@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(+)
[PATCH] virtlogd: solve some memory leaks
Posted by wangjian 3 years, 10 months ago 
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@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)
+{
+    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);
+
     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);
         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
Re: [PATCH] virtlogd: solve some memory leaks
Posted by Andrea Bolognani 3 years, 10 months ago 
On Tue, 2020-06-16 at 14:26 +0800, 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.

Please don't CC random developers when posting patches, or send the
same patch twice. See [1] for more information.

If your patch has been sitting on the list for more than a week or
so, you can ping the list by replying to the original message (or the
cover letter for a multi-patch series).

I'll leave the actual review to someone with more experience in this
area of libvirt.


[1] https://libvirt.org/submitting-patches.html
-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] virtlogd: solve some memory leaks
Posted by Ján Tomko 3 years, 10 months ago 
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@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
>
Re: [PATCH] virtlogd: solve some memory leaks
Posted by Michal Privoznik 3 years, 10 months ago 
On 6/16/20 8:26 AM, 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)  ??:?
> 

Hey,

firstly, I'm interested how you used asan. By providing 
-fsanitize=address to CFLAGS?

Secondly, can you test the latest master? The memleaks you attach show 
libvirt-3.2.0. I've ran valgrind over the latest master but was unable 
to reproduce any leak:

==107012==    definitely lost: 0 bytes in 0 blocks
==107012==    indirectly lost: 0 bytes in 0 blocks
==107012==      possibly lost: 1,368 bytes in 19 blocks

What do I need to do to reproduce?

Michal
RE: [PATCH] virtlogd: solve some memory leaks
Posted by wangjian (AN) 3 years, 10 months ago 
1. Yes, our testers opened some compilation options, such as -fsanitizer=leak.
2. The version of libvirt we are using is 3.2.0. Our tester deployed a long-lived environment and did many operations (he didn't remember what he did), 
  such as restarting virtlogd, restarting libvirtd, creating virtual machines, kill -9 virtlogd, libvirtd, etc.

-----Original Message-----
From: Michal Privoznik [mailto:mprivozn@redhat.com] 
Sent: Tuesday, June 16, 2020 9:58 PM
To: wangjian (AN) <wangjian161@huawei.com>
Cc: libvir-list@redhat.com
Subject: Re: [PATCH] virtlogd: solve some memory leaks

On 6/16/20 8:26 AM, 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)  ??:?
> 

Hey,

firstly, I'm interested how you used asan. By providing -fsanitize=address to CFLAGS?

Secondly, can you test the latest master? The memleaks you attach show libvirt-3.2.0. I've ran valgrind over the latest master but was unable to reproduce any leak:

==107012==    definitely lost: 0 bytes in 0 blocks
==107012==    indirectly lost: 0 bytes in 0 blocks
==107012==      possibly lost: 1,368 bytes in 19 blocks

What do I need to do to reproduce?

Michal
Re: [PATCH] virtlogd: solve some memory leaks
Posted by Michal Privoznik 3 years, 10 months ago 
On 6/17/20 9:58 AM, wangjian (AN) wrote:
> 
> 1. Yes, our testers opened some compilation options, such as -fsanitizer=leak.
> 2. The version of libvirt we are using is 3.2.0. Our tester deployed a long-lived environment and did many operations (he didn't remember what he did),
>    such as restarting virtlogd, restarting libvirtd, creating virtual machines, kill -9 virtlogd, libvirtd, etc.

I've tried -fsanitize=leak but the only issues it found were some tests 
failing because of unexpected retvals from virsh (because asan changes 
that if a memleak was found).

Anyway, I haven't found any leaks inside the virtlogd. The thing is, if 
you found memleaks in 3.2.0 that's a shame, but it's likely that those 
are fixed upstream already. I mean, 3.2.0 is three years old. Therefore, 
we need to prove that these memleaks can happen with more recent 
versions and it's not obvious from the code, sorry.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.