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
