Re: [PATCH] Stop linking tests/commandhelper to libvirt code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 20, 2017 at 12:08:59PM +0100, Daniel P. Berrange wrote:
> The commandhelper binary is a helper for commandtest that
> validates what file handles were inherited. For this to
> work reliably we must not have any libraries that leak
> file descriptors into commandhelper. Unfortunately some
> versions of gnutls will intentionally open file handles
> at library load time via a constructor function.
> 
> We previously hacked around this in
> 
>   commit 4cbc15d037e1cd8abf5c4aa6acc30d83ae13e34d
>   Author: Martin Kletzander <mkletzan@xxxxxxxxxx>
>   Date:   Fri May 2 09:55:52 2014 +0200
> 
>     tests: don't fail with newer gnutls
> 
>     gnutls-3.3.0 and newer leaves 2 FDs open in order to be backwards
>     compatible when it comes to chrooted binaries [1].  Linking
>     commandhelper with gnutls then leaves these two FDs open and
>     commandtest fails thanks to that.  This patch does not link
>     commandhelper with libvirt.la, but rather only the utilities making
>     the test pass.
> 
>     Based on suggestion from Daniel [2].
> 
>     [1] http://lists.gnutls.org/pipermail/gnutls-help/2014-April/003429.html
>     [2] https://www.redhat.com/archives/libvir-list/2014-April/msg01119.html
> 
> That fix relied on fact that while libvirt.so linked with
> gnutls, libvirt_util.la did not link to it.  With the
> introduction of the util/vircrypto.c file that assumption
> is no longer valid. We must not link to libvirt_util.la
> at all - only gnulib and libc can (hopefully) be relied
> on not to open random file descriptors in constructors.
> 
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---

BTW, technically a build breaker fix for the CI problems that are fallout
from my previous CI build breaker fix. Leaving this for review before
pushing since it is a somewhat larger fix.

>  cfg.mk                |  8 ++++----
>  tests/Makefile.am     |  6 ++++--
>  tests/commandhelper.c | 29 +++++++++++++----------------
>  3 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 56cb14b..f2a053f 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -1111,7 +1111,7 @@ $(srcdir)/src/admin/admin_client.h: $(srcdir)/src/admin/admin_protocol.x
>  exclude_file_name_regexp--sc_avoid_strcase = ^tools/vsh\.h$$
>  
>  _src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|xen/xend_internal|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon
> -_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock
> +_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock|commandhelper
>  exclude_file_name_regexp--sc_avoid_write = \
>    ^(src/($(_src1))|daemon/libvirtd|tools/virsh-console|tests/($(_test1)))\.c$$
>  
> @@ -1146,10 +1146,10 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
>    ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>  
>  exclude_file_name_regexp--sc_prohibit_strdup = \
> -  ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
> +  ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$)
>  
>  exclude_file_name_regexp--sc_prohibit_close = \
> -  (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
> +  (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c|tests/commandhelper\.c)$$)
>  
>  exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
>    (^tests/(qemuhelp|virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$)
> @@ -1173,7 +1173,7 @@ exclude_file_name_regexp--sc_prohibit_select = \
>  	^cfg\.mk$$
>  
>  exclude_file_name_regexp--sc_prohibit_raw_allocation = \
> -  ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock)\.c|tools/wireshark/src/packet-libvirt\.c)$$
> +  ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c)$$
>  
>  exclude_file_name_regexp--sc_prohibit_readlink = \
>    ^src/(util/virutil|lxc/lxc_container)\.c$$
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 8138885..0b2305d 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -946,12 +946,14 @@ commandtest_SOURCES = \
>  	commandtest.c testutils.h testutils.c
>  commandtest_LDADD = $(LDADDS)
>  
> +# Must not link to any libvirt modules - libc / gnulib only
> +# otherwise external libraries might unexpectedly leak
> +# file descriptors into commandhelper invalidating the
> +# test logic assumptions
>  commandhelper_SOURCES = \
>  	commandhelper.c
>  commandhelper_LDADD = \
>  	$(NO_INDIRECT_LDFLAGS) \
> -	$(PROBES_O) \
> -	../src/libvirt_util.la \
>  	$(GNULIB_LIBS)
>  
>  commandhelper_LDFLAGS = -static
> diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> index 015efda..e9e6353 100644
> --- a/tests/commandhelper.c
> +++ b/tests/commandhelper.c
> @@ -28,11 +28,6 @@
>  #include <sys/stat.h>
>  
>  #include "internal.h"
> -#include "virutil.h"
> -#include "viralloc.h"
> -#include "virfile.h"
> -#include "testutils.h"
> -#include "virstring.h"
>  
>  #ifndef WIN32
>  
> @@ -50,11 +45,13 @@ static int envsort(const void *a, const void *b)
>      char *bkey;
>      int ret;
>  
> -    ignore_value(VIR_STRNDUP_QUIET(akey, astr, aeq - astr));
> -    ignore_value(VIR_STRNDUP_QUIET(bkey, bstr, beq - bstr));
> +    if (!(akey = strndup(astr, aeq - astr)))
> +        abort();
> +    if (!(bkey = strndup(bstr, beq - bstr)))
> +        abort();
>      ret = strcmp(akey, bkey);
> -    VIR_FREE(akey);
> -    VIR_FREE(bkey);
> +    free(akey);
> +    free(bkey);
>      return ret;
>  }
>  
> @@ -80,8 +77,8 @@ int main(int argc, char **argv) {
>          origenv++;
>      }
>  
> -    if (VIR_ALLOC_N_QUIET(newenv, n) < 0)
> -        goto cleanup;
> +    if (!(newenv = malloc(sizeof(*newenv) * n)))
> +        abort();
>  
>      origenv = environ;
>      n = i = 0;
> @@ -120,7 +117,7 @@ int main(int argc, char **argv) {
>          STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata"))
>          strcpy(cwd, ".../commanddata");
>      fprintf(log, "CWD:%s\n", cwd);
> -    VIR_FREE(cwd);
> +    free(cwd);
>  
>      fprintf(log, "UMASK:%04o\n", umask(0));
>  
> @@ -144,9 +141,9 @@ int main(int argc, char **argv) {
>              goto cleanup;
>          if (got == 0)
>              break;
> -        if (safewrite(STDOUT_FILENO, buf, got) != got)
> +        if (write(STDOUT_FILENO, buf, got) != got)
>              goto cleanup;
> -        if (safewrite(STDERR_FILENO, buf, got) != got)
> +        if (write(STDERR_FILENO, buf, got) != got)
>              goto cleanup;
>      }
>  
> @@ -158,8 +155,8 @@ int main(int argc, char **argv) {
>      ret = EXIT_SUCCESS;
>  
>   cleanup:
> -    VIR_FORCE_FCLOSE(log);
> -    VIR_FREE(newenv);
> +    fclose(log);
> +    free(newenv);
>      return ret;
>  }
>  
> -- 
> 2.5.5
> 

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux