Libguestfs - Dec 2018 - Re: [PATCH nbdkit 0/4] Multiple valgrind improvements and possible security fix.

I worked out why valgrind wasn't being applied to nbdkit when run by
many of the tests (patches 1-2).  Unfortunately I'm not able to make
it actually fail tests when valgrind fails.  Although the situation is
marginally improved in that you can now manually examine the *.log
files and find valgrind failures that way.  Also adds valgrinding of
the Python plugin (patch 3).

Along the way I found that when we create a TLS session object we
never free it, which is a bit of a problem (although easy to fix -
patch 4).

I'll need to backport this fix to every stable branch.  It's not clear
how exploitable this is -- it's my feeling that you'd need to open
millions of TLS sessions which would take forever, and the result
would only be a denial of service as nbdkit runs out of memory and
crashes.

Rich.

Valgrind has two ways to control valgrinding into subprocesses,
--child-silent-after-fork and --trace-children.

--child-silent-after-fork=yes causes tracing to stop when the process
forks.  However in nbdkit we want to continue tracing nbdkit when it
forks itself into the background, so I have removed this option now.

--trace-children=no causes tracing to stop when the program calls one
of the exec(2) functions.  For nbdkit we want this function since we
don't want to trace into subprocesses (eg. when using
nbdkit-sh-plugin).

Note that although we are now tracing into the subprocess, the tests
will still not exit with error if the subprocess leaks memory because
we would need to properly ‘wait -n’ for the subprocess, which
tests/functions.sh does not do.  However a message about the leak will
still get printed in the log.
---
 wrapper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/wrapper.c b/wrapper.c
index e5f74a2..eb50108 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -133,7 +133,6 @@ main (int argc, char *argv[])
     passthru ("--error-exitcode=119");
     passthru_format ("--suppressions=%s/valgrind-suppressions",
srcdir);
     passthru ("--trace-children=no");
-    passthru ("--child-silent-after-fork=yes");
     passthru ("--run-libc-freeres=no");
     passthru ("--num-callers=20");
   }
-- 
2.19.0.rc0

By default valgrind suppresses many leaks.  I'm not even sure exactly
how it decides which ones to suppress, but certainly global variables
pointing to malloc’d data are suppressed, which is not useful
behaviour.

Tell valgrind to show all leaks.  It won't give an error on them.

However to do this we also need a much more comprehensive list of
suppressions so that we don't constantly show unavoidable leaks in
(eg) dlopen.  This adds separate files for each class of suppressions
in a new valgrind/ directory.
---
 .gitignore                                    |   1 +
 Makefile.am                                   |   2 +-
 configure.ac                                  |   3 +-
 valgrind/Makefile.am                          |  47 ++++++++
 valgrind/glibc.suppressions                   | 106 ++++++++++++++++++
 .../nbdkit.suppressions                       |  21 +---
 valgrind/perl.suppressions                    |  40 +++++++
 wrapper.c                                     |   3 +-
 8 files changed, 205 insertions(+), 18 deletions(-)

diff --git a/.gitignore b/.gitignore
index 612c544..86ef6cb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -97,3 +97,4 @@ Makefile.in
 /tests/test-xz
 /tests/test-xz-curl
 /test-driver
+/valgrind/suppressions
diff --git a/Makefile.am b/Makefile.am
index 2b5d80d..a06e683 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -40,7 +40,6 @@ EXTRA_DIST = \
 	html/pod.css \
 	LICENSE \
 	OTHER_PLUGINS \
-	valgrind-suppressions \
 	m4/.gitignore
 
 CLEANFILES += html/*.html
@@ -60,6 +59,7 @@ nbdkit_DEPENDENCIES = config.status
 SUBDIRS = \
 	bash \
 	docs \
+	valgrind \
 	include \
 	common/include \
 	src
diff --git a/configure.ac b/configure.ac
index a3e4457..cf931cd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -834,6 +834,7 @@ AC_CONFIG_FILES([Makefile
                  src/Makefile
                  src/nbdkit.pc
                  tests/functions.sh
-                 tests/Makefile])
+                 tests/Makefile
+                 valgrind/Makefile])
 
 AC_OUTPUT
diff --git a/valgrind/Makefile.am b/valgrind/Makefile.am
new file mode 100644
index 0000000..7312b79
--- /dev/null
+++ b/valgrind/Makefile.am
@@ -0,0 +1,47 @@
+# nbdkit
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+include $(top_srcdir)/common-rules.mk
+
+suppressions_files = $(wildcard *.suppressions)
+
+EXTRA_DIST = $(suppressions_files)
+
+noinst_DATA = suppressions
+
+suppressions: $(suppressions_files)
+	rm -f $@ $@-t
+	cat $^ > $@-t
+	mv $@-t $@
+	chmod 0444 $@
+
+DISTCLEANFILES = suppressions
diff --git a/valgrind/glibc.suppressions b/valgrind/glibc.suppressions
new file mode 100644
index 0000000..6be59ac
--- /dev/null
+++ b/valgrind/glibc.suppressions
@@ -0,0 +1,106 @@
+# glibc valgrind suppressions
+# Copyright (C) 2016-2018 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Allow thread-local storage from pthread_create to leak.
+{
+  glibc_1
+  Memcheck:Leak
+  fun:calloc
+  ...
+  fun:_dl_allocate_tls
+}
+
+# Suppress leaks from dlopen.  When running under valgrind we
+# deliberately don't run dlclose because otherwise valgrind cannot
+# print symbols.  So it's expected that dlopen will leak.
+{
+  glibc_2
+  Memcheck:Leak
+  ...
+  obj:/usr/lib*/libdl-*
+}
+
+# bindtextdomain leaks.
+{
+  glibc_3
+  Memcheck:Leak
+  ...
+  fun:bindtextdomain
+}
+
+# Both gmtime_r and locatime_r leak some sort of timezone- related
+# struct inside glibc.  These are used by the log filter and the
+# floppy plugin.
+{
+  glibc_4
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:tzset_internal
+}
+
+# __printf_chk leaks.
+{
+  glibc_5
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:__printf_chk
+}
+
+# iconv is very leaky, even if we close the handle.
+{
+  glibc_6
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:__gconv_open
+}
+
+{
+  glibc_7
+  Memcheck:Leak
+  fun:calloc
+  ...
+  fun:__gconv_open
+}
+
+# getaddrinfo leaks a memory allocation even though we
+# call freeaddrinfo.
+{
+  glibc_8
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:__check_pf
+  fun:getaddrinfo
+}
diff --git a/valgrind-suppressions b/valgrind/nbdkit.suppressions
similarity index 94%
rename from valgrind-suppressions
rename to valgrind/nbdkit.suppressions
index 671cd79..094d106 100644
--- a/valgrind-suppressions
+++ b/valgrind/nbdkit.suppressions
@@ -1,5 +1,5 @@
 # nbdkit valgrind suppressions
-# Copyright (C) 2016 Red Hat Inc.
+# Copyright (C) 2016-2018 Red Hat Inc.
 # All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
@@ -50,22 +50,13 @@
   fun:allocate_stack
 }
 
-# Allow thread-local storage from pthread_create to leak.
-{
-  nbdkit_3
-  Memcheck:Leak
-  fun:calloc
-  ...
-  fun:_dl_allocate_tls
-}
-
 # close methods are not guaranteed to be called if a connection is in
 # progress when the server is being shut down, so leaks in various
 # *_open functions are fairly inevitable.  We simply have to check by
 # hand that the following leaks are not possible under normal
 # circumstances, and then add them to this list.
 {
-  nbdkit_4
+  nbdkit_3
   Memcheck:Leak
   fun:malloc
   fun:error_open
@@ -73,7 +64,7 @@
 }
 
 {
-  nbdkit_5
+  nbdkit_4
   Memcheck:Leak
   fun:malloc
   fun:file_open
@@ -81,7 +72,7 @@
 }
 
 {
-  nbdkit_6
+  nbdkit_5
   Memcheck:Leak
   fun:malloc
   fun:memory_open
@@ -89,7 +80,7 @@
 }
 
 {
-  nbdkit_7
+  nbdkit_6
   Memcheck:Leak
   fun:malloc
   fun:null_open
@@ -97,7 +88,7 @@
 }
 
 {
-  nbdkit_8
+  nbdkit_7
   Memcheck:Leak
   fun:malloc
   fun:partition_open
diff --git a/valgrind/perl.suppressions b/valgrind/perl.suppressions
new file mode 100644
index 0000000..57d1fcc
--- /dev/null
+++ b/valgrind/perl.suppressions
@@ -0,0 +1,40 @@
+# Perl valgrind suppressions
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Perl leaks lots of memory by design.
+{
+  perl_1
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:perl_construct
+}
diff --git a/wrapper.c b/wrapper.c
index eb50108..3fd499b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -130,8 +130,9 @@ main (int argc, char *argv[])
     passthru (VALGRIND);
     passthru ("--vgdb=no");
     passthru ("--leak-check=full");
+    passthru ("--show-leak-kinds=all");
     passthru ("--error-exitcode=119");
-    passthru_format ("--suppressions=%s/valgrind-suppressions",
srcdir);
+    passthru_format ("--suppressions=%s/valgrind/suppressions",
builddir);
     passthru ("--trace-children=no");
     passthru ("--run-libc-freeres=no");
     passthru ("--num-callers=20");
-- 
2.19.0.rc0

I had to add several suppressions because Python leaks memory by
design, and also increase the max depth of valgrind stack traces
because Python stack traces are very deep.

Note that to do proper valgrinding of Python itself we would need to
specially recompile Python:
https://svn.python.org/projects/python/trunk/Misc/README.valgrind
---
 tests/test-lang-plugins.c    |  3 +-
 valgrind/python.suppressions | 75 ++++++++++++++++++++++++++++++++++++
 wrapper.c                    |  2 +-
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/tests/test-lang-plugins.c b/tests/test-lang-plugins.c
index af926f6..d0ebab3 100644
--- a/tests/test-lang-plugins.c
+++ b/tests/test-lang-plugins.c
@@ -57,8 +57,7 @@ main (int argc, char *argv[])
    */
   s = getenv ("NBDKIT_VALGRIND");
   if (s && strcmp (s, "1") == 0 &&
-      (strcmp (LANG, "python") == 0 ||
-       strcmp (LANG, "ruby") == 0 ||
+      (strcmp (LANG, "ruby") == 0 ||
        strcmp (LANG, "tcl") == 0)) {
     fprintf (stderr, "%s test skipped under valgrind.\n", LANG);
     exit (77);                  /* Tells automake to skip the test. */
diff --git a/valgrind/python.suppressions b/valgrind/python.suppressions
new file mode 100644
index 0000000..bd20abe
--- /dev/null
+++ b/valgrind/python.suppressions
@@ -0,0 +1,75 @@
+# Python valgrind suppressions
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Python leaks lots of memory by design.
+{
+  python_1
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:Py_InitializeEx
+}
+
+{
+  python_2
+  Memcheck:Leak
+  fun:calloc
+  ...
+  fun:Py_InitializeEx
+}
+
+{
+  python_3
+  Memcheck:Leak
+  fun:realloc
+  ...
+  fun:Py_InitializeEx
+}
+
+# As far as I can tell there is no way to stop the Python object
+# (representing the compiled code?) from leaking from this API.
+{
+  python_4
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:PyRun_SimpleFileExFlags
+}
+
+# This seems like a bug in Python itself.
+{
+  python_5
+  Memcheck:Leak
+  fun:malloc
+  ...
+  fun:Py_Finalize
+}
diff --git a/wrapper.c b/wrapper.c
index 3fd499b..f19c09a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -135,7 +135,7 @@ main (int argc, char *argv[])
     passthru_format ("--suppressions=%s/valgrind/suppressions",
builddir);
     passthru ("--trace-children=no");
     passthru ("--run-libc-freeres=no");
-    passthru ("--num-callers=20");
+    passthru ("--num-callers=100");
   }
   else {
     s = getenv ("NBDKIT_GDB");
-- 
2.19.0.rc0

This structure was not freed along the non-error path, both resulting
in a memory leak and providing an easy way for clients to blow up
nbdkit servers if they enable TLS support.  Ooops.

Found by valgrind.
---
 src/crypto.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/crypto.c b/src/crypto.c
index 948e79e..c2f9971 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -391,6 +391,10 @@ crypto_close (struct connection *conn)
     close (sockin);
   if (sockout >= 0 && sockin != sockout)
     close (sockout);
+
+  gnutls_deinit (*session);
+  free (session);
+  connection_set_crypto_session (conn, NULL);
 }
 
 /* Upgrade an existing connection to TLS.  Also this should do access
@@ -505,6 +509,7 @@ crypto_negotiate_tls (struct connection *conn, int sockin,
int sockout)
  error:
   gnutls_deinit (*session);
   free (session);
+  connection_set_crypto_session (conn, NULL);
   return -1;
 }
 
-- 
2.19.0.rc0

I think attached is a better version of this patch.  In particular it
avoids setting the per-connection data until we're at the very end of
the initialization function.

Technically this is not part of the fix for the memory leak, but
nevertheless this avoids any possible case where we might call
crypto_close without a valid session along some error path.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> Valgrind has two ways to control valgrinding into subprocesses,
> --child-silent-after-fork and --trace-children.
> 
> --child-silent-after-fork=yes causes tracing to stop when the process
> forks.  However in nbdkit we want to continue tracing nbdkit when it
> forks itself into the background, so I have removed this option now.
> 
> --trace-children=no causes tracing to stop when the program calls one
> of the exec(2) functions.  For nbdkit we want this function since we
> don't want to trace into subprocesses (eg. when using
> nbdkit-sh-plugin).
> 
> Note that although we are now tracing into the subprocess, the tests
> will still not exit with error if the subprocess leaks memory because
> we would need to properly ‘wait -n’ for the subprocess, which
> tests/functions.sh does not do.  However a message about the leak will
> still get printed in the log.
> ---
>   wrapper.c | 1 -
>   1 file changed, 1 deletion(-)
ACK

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

On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> By default valgrind suppresses many leaks.  I'm not even sure exactly
> how it decides which ones to suppress, but certainly global variables
> pointing to malloc’d data are suppressed, which is not useful
> behaviour.
Here's my understanding of why that is the default - if you assign 
malloc()d storage into a global, then that storage is in scope right up 
until you call exit().  And it's faster (and often easier) to exit() a 
process with memory still allocated (the OS will clean it up anyways) 
than it is to manually chase down the memory cleanups yourself.
> 
> Tell valgrind to show all leaks.  It won't give an error on them.
> 
> However to do this we also need a much more comprehensive list of
> suppressions so that we don't constantly show unavoidable leaks in
> (eg) dlopen.  This adds separate files for each class of suppressions
> in a new valgrind/ directory.
> ---
>   .gitignore                                    |   1 +
>   Makefile.am                                   |   2 +-
>   configure.ac                                  |   3 +-
>   valgrind/Makefile.am                          |  47 ++++++++
>   valgrind/glibc.suppressions                   | 106 ++++++++++++++++++
>   .../nbdkit.suppressions                       |  21 +---
>   valgrind/perl.suppressions                    |  40 +++++++
>   wrapper.c                                     |   3 +-
>   8 files changed, 205 insertions(+), 18 deletions(-)
> 
> +++ b/valgrind/Makefile.am
> @@ -0,0 +1,47 @@
> +include $(top_srcdir)/common-rules.mk
> +
> +suppressions_files = $(wildcard *.suppressions)
A GNU make-ism - but you already mention requiring GNU make in README. 
Should we make ./configure error out hard if $MAKE is not GNU Make, 
rather than risking someone getting 80% though a build on BSD make and 
then choking when it gets here?
> +++ b/wrapper.c
> @@ -130,8 +130,9 @@ main (int argc, char *argv[])
>       passthru (VALGRIND);
>       passthru ("--vgdb=no");
>       passthru ("--leak-check=full");
> +    passthru ("--show-leak-kinds=all");
I could understand this if we were implementing a library and wanted to 
make it easier for some other user to call our library shutdown to 
reclaim all memory that we otherwise stashed in globals - but when we 
are just a standalone app, do we really need to worry about memory still 
reachable in globals, as that's not a true leak?
>       passthru ("--error-exitcode=119");
> -    passthru_format ("--suppressions=%s/valgrind-suppressions",
srcdir);
> +    passthru_format ("--suppressions=%s/valgrind/suppressions",
builddir);
Is this still right under VPATH?
>       passthru ("--trace-children=no");
>       passthru ("--run-libc-freeres=no");
>       passthru ("--num-callers=20");
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> I had to add several suppressions because Python leaks memory by
> design, and also increase the max depth of valgrind stack traces
> because Python stack traces are very deep.
> 
> Note that to do proper valgrinding of Python itself we would need to
> specially recompile Python:
> https://svn.python.org/projects/python/trunk/Misc/README.valgrind
Odd - the site asks for username/password, but still shows the content 
even when I cancel the dialog box.
> ---
>   tests/test-lang-plugins.c    |  3 +-
>   valgrind/python.suppressions | 75 ++++++++++++++++++++++++++++++++++++
>   wrapper.c                    |  2 +-
>   3 files changed, 77 insertions(+), 3 deletions(-)
ACK.

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

On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> I worked out why valgrind wasn't being applied to nbdkit when run by
> many of the tests (patches 1-2).  Unfortunately I'm not able to make
> it actually fail tests when valgrind fails.  Although the situation is
> marginally improved in that you can now manually examine the *.log
> files and find valgrind failures that way.  Also adds valgrinding of
> the Python plugin (patch 3).
> 
> Along the way I found that when we create a TLS session object we
> never free it, which is a bit of a problem (although easy to fix -
> patch 4).
> 
> I'll need to backport this fix to every stable branch.  It's not
clear
> how exploitable this is -- it's my feeling that you'd need to open
> millions of TLS sessions which would take forever, and the result
> would only be a denial of service as nbdkit runs out of memory and
> crashes.
Can the leak happen with merely a port probe, or only by someone that 
was able to get past the handshake?  If the former, then it is a vehicle 
for DoS attacks and probably worth a CVE (because the person performing 
the port probe can crash nbdkit from servicing real clients, even though 
the attacker does not own TLS credentials); if the latter, it is not a 
security bug (no escalation in privilege by locking yourself out).

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

On Mon, Dec 03, 2018 at 01:03:07PM -0600, Eric Blake
wrote:
> On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> >I worked out why valgrind wasn't being applied to nbdkit when run
by
> >many of the tests (patches 1-2).  Unfortunately I'm not able to
make
> >it actually fail tests when valgrind fails.  Although the situation is
> >marginally improved in that you can now manually examine the *.log
> >files and find valgrind failures that way.  Also adds valgrinding of
> >the Python plugin (patch 3).
> >
> >Along the way I found that when we create a TLS session object we
> >never free it, which is a bit of a problem (although easy to fix -
> >patch 4).
> >
> >I'll need to backport this fix to every stable branch.  It's
not clear
> >how exploitable this is -- it's my feeling that you'd need to
open
> >millions of TLS sessions which would take forever, and the result
> >would only be a denial of service as nbdkit runs out of memory and
> >crashes.
> 
> Can the leak happen with merely a port probe, or only by someone
> that was able to get past the handshake?  If the former, then it is
> a vehicle for DoS attacks and probably worth a CVE (because the
> person performing the port probe can crash nbdkit from servicing
> real clients, even though the attacker does not own TLS
> credentials);
We allocate the session here:

https://github.com/libguestfs/nbdkit/blob/dae18932153d7249fe74bfcec893115d1f006793/src/crypto.c#L408

Along every error path the function frees the session.

The handshake happens here:

https://github.com/libguestfs/nbdkit/blob/dae18932153d7249fe74bfcec893115d1f006793/src/crypto.c#L494

If it fails we jump to the ‘error:’ label which will free the session.
The leak only happens if we handshake successfully, in which case the
function returns 0 and the session is never freed after that.
> if the latter, it is not a security bug (no escalation
> in privilege by locking yourself out).
It's possible to run a public facing nbdkit server with TLS which does
not check client certificates nor PSKs (in fact, that's the default).
So it's likely a DoS.  My reason for saying it's low priority is that
you'd need to initiate quite a lot of TLS sessions to leak any
significant amount of memory.

According to valgrind each session leaks 13,996 bytes.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org