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@xxxxxxxxxx>
---
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
Attachment:
signature.asc
Description: PGP signature
