Message ID | 20150304212323.43B2E2C3B7B@topped-with-meat.com |
---|---|
State | New |
Headers | show |
On 03/04/2015 10:23 PM, Roland McGrath wrote: > This extends the makefile infrastructure for tests to use a variable > <testname>-ENV-only that is like the existing <testname>-ENV but for > cases where the test program should run with a completely bare > environment except for the explicit assignments in the makefile. > This lets us get rid of the use of execve in bug-setlocale1 (and any > similar cases I have not yet noticed). Some things break if you run them with certain locale settings, so some tests lack this environment isolation. I wanted to sanitize the environment more thoroughly, for all tests: <https://sourceware.org/ml/libc-alpha/2014-09/msg00100.html> I haven't implemented the env -u suggestion yet.
On 04 Mar 2015 13:23, Roland McGrath wrote: > This extends the makefile infrastructure for tests to use a variable > <testname>-ENV-only that is like the existing <testname>-ENV but for > cases where the test program should run with a completely bare > environment except for the explicit assignments in the makefile. > This lets us get rid of the use of execve in bug-setlocale1 (and any > similar cases I have not yet noticed). lgtm -mike
I suspect that where we want to get to is that all tests run with a completely controlled environment. I can't really think of any reason why any of our tests should be run with random ambient environment variables. But I'm starting small.
On 03/05/2015 05:18 PM, Roland McGrath wrote: > I suspect that where we want to get to is that all tests run with a > completely controlled environment. I can't really think of any reason why > any of our tests should be run with random ambient environment variables. > But I'm starting small. Agreed. Cheers, Carlos.
On Wed, 4 Mar 2015, Roland McGrath wrote: > ifndef test-wrapper-env > test-wrapper-env = $(test-wrapper) env > endif > +# Likewise, but the program's environment will be empty except for any > +# explicit <variable>=<value> assignments preceding the program name. > +ifndef test-wrapper-env-only > +test-wrapper-env-only = $(test-wrapper) env -i > +endif Needs documenting in install.texi / INSTALL, where it describes how test-wrapper-env must be set if $(test-wrapper) env isn't sufficient.
On Thu, 5 Mar 2015, Roland McGrath wrote: > I suspect that where we want to get to is that all tests run with a > completely controlled environment. I can't really think of any reason why > any of our tests should be run with random ambient environment variables. > But I'm starting small. Test scripts run on the build system certainly need variables such as PATH to find utilities. It may be that all test programs run on glibc's host never need to execute any external utilities, so don't need PATH. Some tests may create temporary files in TMPDIR, though use of /tmp should be fine as long as they use unique names (and if they don't, this needs fixing - bug 13888). Some tests (e.g. glob expanding ~) depend on HOME, but setting values local to the test directory may work. Those seem the most likely environment variable dependencies for tests.
> Test scripts run on the build system certainly need variables such as PATH > to find utilities. Sure. Entirely controlled doesn't mean entirely empty.
On 06 Mar 2015 18:52, Joseph Myers wrote: > On Thu, 5 Mar 2015, Roland McGrath wrote: > > I suspect that where we want to get to is that all tests run with a > > completely controlled environment. I can't really think of any reason why > > any of our tests should be run with random ambient environment variables. > > But I'm starting small. > > Test scripts run on the build system certainly need variables such as PATH > to find utilities. > > It may be that all test programs run on glibc's host never need to execute > any external utilities, so don't need PATH. Some tests may create > temporary files in TMPDIR, though use of /tmp should be fine as long as > they use unique names (and if they don't, this needs fixing - bug 13888). > Some tests (e.g. glob expanding ~) depend on HOME, but setting values > local to the test directory may work. Those seem the most likely > environment variable dependencies for tests. wrt TMPDIR, it would be nice if the test runner managed a tempdir for the test. that way we don't have to worry: - whether the test properly cleans up after itself on exit/crash/interrupt; we can just assume it's always done for us and thus not have to even try - are we using the TMPDIR securely this could be done to a degree in the test-skeleton -mike
--- a/Makeconfig +++ b/Makeconfig @@ -611,6 +611,11 @@ endif ifndef test-wrapper-env test-wrapper-env = $(test-wrapper) env endif +# Likewise, but the program's environment will be empty except for any +# explicit <variable>=<value> assignments preceding the program name. +ifndef test-wrapper-env-only +test-wrapper-env-only = $(test-wrapper) env -i +endif # Whether to run test programs built for the library's host system. ifndef run-built-tests --- a/Rules +++ b/Rules @@ -186,9 +186,11 @@ ifneq "$(strip $(tests) $(xtests) $(test-srcs))" "" # These are the implicit rules for making test outputs # from the test programs and whatever input files are present. -make-test-out = $(test-wrapper-env) \ - $(run-program-env) \ - $($*-ENV) $(host-test-program-cmd) $($*-ARGS) +define make-test-out +$(if $($*-ENV-only),$(test-wrapper-env-only) $($*-ENV-only),\ + $(test-wrapper-env) $(run-program-env) $($*-ENV)) \ +$(host-test-program-cmd) $($*-ARGS) +endef $(objpfx)%.out: %.input $(objpfx)% $(make-test-out) > $@ < $(word 1,$^); \ $(evaluate-test) --- a/localedata/Makefile +++ b/localedata/Makefile @@ -237,8 +237,8 @@ $(objpfx)mtrace-tst-leaks.out: $(objpfx)tst-leaks.out $(common-objpfx)malloc/mtrace $(objpfx)tst-leaks.mtrace > $@; \ $(evaluate-test) -bug-setlocale1-ARGS = -- $(host-test-program-cmd) -bug-setlocale1-static-ARGS = $(bug-setlocale1-ARGS) +bug-setlocale1-ENV-only = LOCPATH=$(objpfx) LC_CTYPE=de_DE.UTF-8 +bug-setlocale1-static-ENV-only = $(bug-setlocale1-ENV-only) $(objdir)/iconvdata/gconv-modules: $(MAKE) -C ../iconvdata subdir=iconvdata $@ --- a/localedata/bug-setlocale1.c +++ b/localedata/bug-setlocale1.c @@ -7,44 +7,8 @@ static int -do_test (int argc, char *argv[]) +do_test (void) { - if (argc > 1) - { - char *newargv[5]; - int i; - if (argc != 2 && argc != 5) - { - printf ("wrong number of arguments (%d)\n", argc); - return 1; - } - - for (i = 0; i < (argc == 5 ? 4 : 1); i++) - newargv[i] = argv[i + 1]; - newargv[i] = NULL; - - char *env[3]; - env[0] = (char *) "LC_CTYPE=de_DE.UTF-8"; - char *loc = getenv ("LOCPATH"); - if (loc == NULL || loc[0] == '\0') - { - puts ("LOCPATH not set"); - return 1; - } - asprintf (&env[1], "LOCPATH=%s", loc); - if (env[1] == NULL) - { - puts ("asprintf failed"); - return 1; - } - env[2] = NULL; - - execve (newargv[0], newargv, env); - - puts ("execve returned"); - return 1; - } - int result = 0; char *a = setlocale (LC_ALL, ""); @@ -128,5 +92,5 @@ do_test (int argc, char *argv[]) return result; } -#define TEST_FUNCTION do_test (argc, argv) +#define TEST_FUNCTION do_test () #include "../test-skeleton.c"