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 :|