Libguestfs - Mar 2019 - Re: [PATCH nbdkit] wrapper: Set MALLOC_CHECK=1 and MALLOC_PERTURB_ (randomly).

This is a cheap way to find some use-after-free and uninitialized read
problems when using glibc.

This in fact reveals a race during filter shutdown (which this
commit does not fix):

 Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)):
 #0  0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from /lib64/libpthread.so.0
 #1  0x0000000000408842 in lock_unload () at locks.c:97
 #2  0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77
 #3  0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at main.c:649

 Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)):
 #0  0x000000000040732a in filter_finalize (b=0x203c330, conn=0x203d870)
     at filters.c:421
 #1  0x0000000000404d07 in _handle_single_connection (sockin=6, sockout=6)
     at connections.c:239
 #2  0x0000000000404d76 in handle_single_connection (sockin=6, sockout=6)
     at connections.c:258
 #3  0x00000000004119f6 in start_thread (datav=0x203b450) at sockets.c:263
 #4  0x00007f1cab09b5a2 in start_thread () from /lib64/libpthread.so.0
 #5  0x00007f1caafc85c3 in clone () from /lib64/libc.so.6

What's happening here is that the filter / plugin chain is freed by
Thread 2, while Thread 1 is calling filter->finalize.  At this point
filter->finalize points to freed memory, but "normally" this would
still contain the correct function pointer.  However when
MALLOC_PERTURB_ is set the function pointer is overwritten with the
non-zero dead memory pattern which we then attempt to call, causing a
segfault (in Thread 1).
---
 wrapper.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index ffb058b..6b5118c 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -70,6 +70,7 @@
 #include <unistd.h>
 #include <getopt.h>
 #include <limits.h>
+#include <time.h>
 
 #include "options.h"
 
@@ -133,6 +134,9 @@ main (int argc, char *argv[])
 {
   bool verbose = false;
   char *s;
+  time_t t;
+  unsigned tu;
+  char ts[32];
 
   /* If NBDKIT_VALGRIND=1 is set in the environment, then we run the
    * program under valgrind.  This is used by the tests.  Similarly if
@@ -250,6 +254,19 @@ main (int argc, char *argv[])
   if (verbose)
     print_command ();
 
+  /* This is a cheap way to find some use-after-free and uninitialized
+   * read problems when using glibc, and doesn't affect normal
+   * operation or other libc.  We don't overwrite existing values so
+   * this can be disabled or overridden at runtime.
+   */
+  setenv ("MALLOC_CHECK_", "1", 0);
+  time (&t);
+  tu = t;
+  tu %= 255;
+  tu++;
+  snprintf (ts, sizeof ts, "%u", tu);
+  setenv ("MALLOC_PERTURB_", ts, 0);
+
   /* Run the final command. */
   execvp (cmd[0], (char **) cmd);
   perror (cmd[0]);
-- 
2.20.1

On 3/18/19 11:59 AM, Richard W.M. Jones wrote:
> This is a cheap way to find some use-after-free and uninitialized read
> problems when using glibc.
> 
> This in fact reveals a race during filter shutdown (which this
> commit does not fix):
> 
>  Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)):
>  #0  0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from
/lib64/libpthread.so.0
>  #1  0x0000000000408842 in lock_unload () at locks.c:97
>  #2  0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77
Do we need some sort of pthread_join() in filter_free() to allow...
>  #3  0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at
main.c:649
> 
>  Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)):
>  #0  0x000000000040732a in filter_finalize (b=0x203c330, conn=0x203d870)
>      at filters.c:421
...filter_finalize() time to finish its job?  But I agree that solving
the race is an independent patch.
>  #1  0x0000000000404d07 in _handle_single_connection (sockin=6, sockout=6)
>      at connections.c:239
>  #2  0x0000000000404d76 in handle_single_connection (sockin=6, sockout=6)
>      at connections.c:258
>  #3  0x00000000004119f6 in start_thread (datav=0x203b450) at sockets.c:263
>  #4  0x00007f1cab09b5a2 in start_thread () from /lib64/libpthread.so.0
>  #5  0x00007f1caafc85c3 in clone () from /lib64/libc.so.6
> 
> What's happening here is that the filter / plugin chain is freed by
> Thread 2, while Thread 1 is calling filter->finalize.  At this point
> filter->finalize points to freed memory, but "normally" this
would
> still contain the correct function pointer.  However when
> MALLOC_PERTURB_ is set the function pointer is overwritten with the
> non-zero dead memory pattern which we then attempt to call, causing a
> segfault (in Thread 1).
> ---
>  wrapper.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
I like that it makes in-tree testing more robust, without penalizing
release builds.

ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Mar 18, 2019 at 12:29:02PM -0500, Eric Blake
wrote:
> On 3/18/19 11:59 AM, Richard W.M. Jones wrote:
> > This is a cheap way to find some use-after-free and uninitialized read
> > problems when using glibc.
> > 
> > This in fact reveals a race during filter shutdown (which this
> > commit does not fix):
> > 
> >  Thread 2 (Thread 0x7f1caaa5ffc0 (LWP 7223)):
> >  #0  0x00007f1cab0a05f8 in pthread_rwlock_wrlock () from
/lib64/libpthread.so.0
> >  #1  0x0000000000408842 in lock_unload () at locks.c:97
> >  #2  0x00000000004066ff in filter_free (b=0x203c330) at filters.c:77
> 
> Do we need some sort of pthread_join() in filter_free() to allow...
> 
> >  #3  0x000000000040a6f4 in main (argc=11, argv=0x7ffc1f4486e8) at
main.c:649
> > 
> >  Thread 1 (Thread 0x7f1caaa5e700 (LWP 7226)):
> >  #0  0x000000000040732a in filter_finalize (b=0x203c330,
conn=0x203d870)
> >      at filters.c:421
> 
> ...filter_finalize() time to finish its job?  But I agree that solving
> the race is an independent patch.
I don't think there's a very simple answer here.  On the other hand
I've long known that there are problems with nbdkit along the signal /
shutdown path, and it needs attention at some point.

Rich.
> >  #1  0x0000000000404d07 in _handle_single_connection (sockin=6,
sockout=6)
> >      at connections.c:239
> >  #2  0x0000000000404d76 in handle_single_connection (sockin=6,
sockout=6)
> >      at connections.c:258
> >  #3  0x00000000004119f6 in start_thread (datav=0x203b450) at
sockets.c:263
> >  #4  0x00007f1cab09b5a2 in start_thread () from /lib64/libpthread.so.0
> >  #5  0x00007f1caafc85c3 in clone () from /lib64/libc.so.6
> > 
> > What's happening here is that the filter / plugin chain is freed
by
> > Thread 2, while Thread 1 is calling filter->finalize.  At this
point
> > filter->finalize points to freed memory, but "normally"
this would
> > still contain the correct function pointer.  However when
> > MALLOC_PERTURB_ is set the function pointer is overwritten with the
> > non-zero dead memory pattern which we then attempt to call, causing a
> > segfault (in Thread 1).
> > ---
> >  wrapper.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> 
> I like that it makes in-tree testing more robust, without penalizing
> release builds.
> 
> ACK.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v