Richard W.M. Jones
2018-Dec-02 16:39 UTC
[Libguestfs] [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.
Richard W.M. Jones
2018-Dec-02 16:39 UTC
[Libguestfs] [PATCH nbdkit 1/4] valgrind: Remove --child-silent-after-fork.
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
Richard W.M. Jones
2018-Dec-02 16:39 UTC
[Libguestfs] [PATCH nbdkit 2/4] valgrind: Add --show-leak-kinds=all and comprehensive list of suppressions.
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
Richard W.M. Jones
2018-Dec-02 16:39 UTC
[Libguestfs] [PATCH nbdkit 3/4] valgrind: Enable valgrinding of Python plugin.
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
Richard W.M. Jones
2018-Dec-02 16:39 UTC
[Libguestfs] [PATCH nbdkit 4/4] crypto: Free TLS session.
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
Richard W.M. Jones
2018-Dec-02 19:19 UTC
Re: [Libguestfs] [PATCH nbdkit 4/4] crypto: Free TLS session.
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
Eric Blake
2018-Dec-03 18:49 UTC
Re: [Libguestfs] [PATCH nbdkit 1/4] valgrind: Remove --child-silent-after-fork.
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
Eric Blake
2018-Dec-03 18:55 UTC
Re: [Libguestfs] [PATCH nbdkit 2/4] valgrind: Add --show-leak-kinds=all and comprehensive list of suppressions.
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
Eric Blake
2018-Dec-03 18:58 UTC
Re: [Libguestfs] [PATCH nbdkit 3/4] valgrind: Enable valgrinding of Python plugin.
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.valgrindOdd - 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
Eric Blake
2018-Dec-03 19:03 UTC
Re: [Libguestfs] [PATCH nbdkit 0/4] Multiple valgrind improvements and possible security fix.
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
Richard W.M. Jones
2018-Dec-03 19:15 UTC
Re: [Libguestfs] [PATCH nbdkit 0/4] Multiple valgrind improvements and possible security fix.
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
Possibly Parallel Threads
- [PATCH nbdkit 0/4] Multiple valgrind improvements and possible security fix.
- [PATCH v2 1/9] build: Remove ./configure --enable-valgrind-daemon.
- [PATCH nbdkit 2/4] valgrind: Add --show-leak-kinds=all and comprehensive list of suppressions.
- [PATCH 1/2] valgrind: Use --trace-children=no --child-silent-after-fork=yes
- [PATCH nbdkit 1/4] valgrind: Remove --child-silent-after-fork.