[libvirt] [PATCH] Stop linking tests/commandhelper to libvirt code
Daniel P. Berrange
berrange at redhat.com
Wed Sep 20 11:31:01 UTC 2017
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 at 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.
>
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
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 :|
More information about the libvir-list
mailing list