Stop linking tests/commandhelper to libvirt code

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@redhat.com>
  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.

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

diff --git a/cfg.mk b/cfg.mk
index 56cb14bd94eccc36c3e46eb856ec063db67b9d0a..f2a053f4b82275c031d0fca1adc2fba65f82c646 100644 (file)
--- 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 813888575c7fd1e84261e05ccd0aff15037c54ed..0b2305d70b6d62a289dd8cbea20862847cd77a52 100644 (file)
--- 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 015efdaa6618f9df42de5b5590bcd67f17eb0c8b..e9e6353f3f525c28c16608dacd0a07ae619c9fbf 100644 (file)
--- 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;
 }