cfg.mk | 8 ++++---- tests/Makefile.am | 6 ++++-- tests/commandhelper.c | 29 +++++++++++++---------------- 3 files changed, 21 insertions(+), 22 deletions(-)
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.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com> Reviewed-by: Martin Kletzander <mkletzan@redhat.com> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.