[PATCH] virthread: Free thread name only after worker has finished
When spawning a thread via our virThread APIs we let pthread spawn this helper thread which sets couple of thread local variables (e.g. thread job name or thread worker name) and as of v6.1.0-40-gc85256b31b it also sets pthread name (which is then visible in `ps' output for instance). Only after these steps the intended function is called. However, just before calling it we free the buffer that holds the thread name which results in invalid memory reads: ==47027== Invalid read of size 1 ==47027== at 0x48389C2: strlen (vg_replace_strmem.c:459) ==47027== by 0x58BB3D6: __vfprintf_internal (vfprintf-internal.c:1645) ==47027== by 0x58CE6E0: __vasprintf_internal (vasprintf.c:57) ==47027== by 0x574BA28: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.6000.7) ==47027== by 0x57240CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.6000.7) ==47027== by 0x48E0EFA: vir_g_strdup_vprintf (glibcompat.c:209) ==47027== by 0x493AA05: virLogVMessage (virlog.c:573) ==47027== by 0x493A8FE: virLogMessage (virlog.c:513) ==47027== by 0x4992FC7: virThreadJobClear (virthreadjob.c:121) ==47027== by 0x4992844: virThreadHelper (virthread.c:237) ==47027== by 0x5817496: start_thread (pthread_create.c:486) ==47027== by 0x59563CE: clone (clone.S:95) The problem is that neither virThreadJobSetWorker() nor virThreadJobSet() create a copy of passed name. They just set a thread local variable to point to the buffer which is then freed. Moving the free towards the end of the wrapper function solves the issue. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virthread.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virthread.c b/src/util/virthread.c index 37b2cdfbe9..64013b575c 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -217,7 +217,6 @@ static void *virThreadHelper(void *data) } else { thname = g_strdup(local.name); } - g_free(local.name); #if defined(__linux__) || defined(WIN32) pthread_setname_np(pthread_self(), thname); @@ -236,6 +235,7 @@ static void *virThreadHelper(void *data) if (!local.worker) virThreadJobClear(0); + g_free(local.name); return NULL; } -- 2.24.1
On Fri, Mar 06, 2020 at 04:22:13PM +0100, Michal Privoznik wrote:
When spawning a thread via our virThread APIs we let pthread spawn this helper thread which sets couple of thread local variables (e.g. thread job name or thread worker name) and as of v6.1.0-40-gc85256b31b it also sets pthread name (which is then visible in `ps' output for instance). Only after these steps the intended function is called. However, just before calling it we free the buffer that holds the thread name which results in invalid memory reads:
==47027== Invalid read of size 1 ==47027== at 0x48389C2: strlen (vg_replace_strmem.c:459) ==47027== by 0x58BB3D6: __vfprintf_internal (vfprintf-internal.c:1645) ==47027== by 0x58CE6E0: __vasprintf_internal (vasprintf.c:57) ==47027== by 0x574BA28: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.6000.7) ==47027== by 0x57240CC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.6000.7) ==47027== by 0x48E0EFA: vir_g_strdup_vprintf (glibcompat.c:209) ==47027== by 0x493AA05: virLogVMessage (virlog.c:573) ==47027== by 0x493A8FE: virLogMessage (virlog.c:513) ==47027== by 0x4992FC7: virThreadJobClear (virthreadjob.c:121) ==47027== by 0x4992844: virThreadHelper (virthread.c:237) ==47027== by 0x5817496: start_thread (pthread_create.c:486) ==47027== by 0x59563CE: clone (clone.S:95)
The problem is that neither virThreadJobSetWorker() nor virThreadJobSet() create a copy of passed name. They just set a thread local variable to point to the buffer which is then freed. Moving the free towards the end of the wrapper function solves the issue.
Doh, I totally didn't expect them to be saving the passed-in pointer, instead of dup'ing it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virthread.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virthread.c b/src/util/virthread.c index 37b2cdfbe9..64013b575c 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -217,7 +217,6 @@ static void *virThreadHelper(void *data) } else { thname = g_strdup(local.name); } - g_free(local.name);
#if defined(__linux__) || defined(WIN32) pthread_setname_np(pthread_self(), thname); @@ -236,6 +235,7 @@ static void *virThreadHelper(void *data) if (!local.worker) virThreadJobClear(0);
+ g_free(local.name); return NULL; }
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik