TEMP and TMP env vars not set in CGI scripts run by LayoutTests (edited) |
|||||||||
Issue description(edited description) I have reviewed a CL [1] involving creating and deleting temporary files in CGI scripts used by LayoutTests. It appears that the CGI scripts' environment misses the TEMP / TMP / TEMPDIR env variables, which conventionally point to a directory that can hold temporary files. We should fix the environment, so individual CGI scripts don't have to carry workarounds for this omission. (original description) I am currently reviewing a CL [1] that involves creating and deleting temporary files. I have also encountered the need for a temporary directory before, e.g. in LevelDB tests. I think the following strategy could prove useful on bots: 1) Before testing a CL, create a new temporary directory. 2) Set the TMP and TEMP env vars (and other synonyms that I've missed) to point to this directory. 3) After the test completes, recursively delete the directory. Being able to depend on these variables will make it easier to write new tests. Also, assuming older tests are migrated over time, I think it'd be nice to have all tests create temporary data in the same place, and make sure it's cleaned up. [1] https://crrev.com/c/755342
,
Nov 16 2017
I wasn't seeing useful values inside the Perl CGI on Win32. On Linux and Mac OS X the variables were set correctly, and File::Temp's tmpdir also did not return a useful location - instead it ended up resolving to \ on the active drive (E:). I worked around it like this:
my $win32 = eval 'use Win32; 1' ? 1 : 0;
# Find the logical equivalent of /tmp - however extra contortions are
# needed on Win32 with tainting and an unpopulated environment (where
# tmpdir() does not work correctly) to avoid choosing the root
# directory of the active drive by default.
my $local_appdata_temp = tmpdir();
if ($win32) {
my $local_appdata = Win32::GetFolderPath(Win32::CSIDL_LOCAL_APPDATA());
if (($local_appdata ne '') && -d "$local_appdata\\Temp") {
$local_appdata_temp = "$local_appdata\\Temp";
}
}
# This fallback order works well on fairly sane "vanilla" Win32, OS X
# and Linux Apache configurations with mod_perl.
my $system_tmpdir =
$ENV{TMPDIR} || $ENV{TEMP} || $ENV{TMP} || $local_appdata_temp;
$system_tmpdir =~ /\A([^\0- ]*)\z/s
or die "untaint failed: $!";
$system_tmpdir = $1;
if ($win32) {
$system_tmpdir =
Win32::GetFullPathName(Win32::GetANSIPathName($system_tmpdir));
}
,
Nov 20 2017
I think this is more of a recipe / swarming question than it is a infra>client>chrome question; maruel@, what are your thoughts on this?
,
Nov 21 2017
The tests are not run by the recipe, they are run as independent Swarming task. This ensures reproducibility and hermeticity, while enabling maximum parallelism. The Swarming bot uses run_isolated.py, which setups its own temporary directory for the duration of the task. It is set as TEMP on Windows and TMPDIR on OSX and Ubuntu. ref: https://cs.chromium.org/chromium/infra/luci/client/run_isolated.py?l=301 Use these variables and you'll be fine. That's what the Chromium tests do by default, I don't know about layout tests.
,
Nov 21 2017
FWIW, those environment variables don't seem to be propagating to the Perl CGI script launched by the web server on Windows. Workaround currently checked in to the file-creation CGI https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi?q=CSIDL_LOCAL_APPDATA+file:%5C.cgi&sq=package:chromium&l=83&dr=C ... and the cleanup CGI: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/delete-temp-file.cgi?q=CSIDL_LOCAL_APPDATA+file:%5C.cgi&sq=package:chromium&l=57&dr=C ... however I would love to remove those and just use tmpdir from File::Spec::Functions https://perldoc.perl.org/File/Spec.html#tmpdir which in the File::Spec::Win32 case is supposed to honor %TEMP% in : https://perldoc.perl.org/File/Spec/Win32.html#tmpdir
,
Nov 21 2017
Is this related? (totally fishing around) https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py?l=164 Since Swarming is doing exactly what the initial request is about, I'll remove the Swarming component. It is neither a recipe issue, since this is about isolated tests.
,
Nov 21 2017
I believe the env vars should be propagating through run-wekbit-tests: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win.py?l=137 The next step would be to try and figure out if they aren't.
,
Nov 27 2017
dpranke@: Thank you very much for routing this! maruel@: Thank you very much for your response! bsittler@: I think the root issue is either a Perl problem, or a problem with how we run Perl. At least on Windows, Perl is ran with taint on [1]. Under this mode, File::Spec's tmpdir ignores the environment variables in Unix [2] (presumably Linux and MacOS?) and Windows [3]. Is it possible that you concluded the environment variables are not set because you attempted to use tmpdir and got the fallback paths? If so, can you please update the comments in write-temp-file.cgi [4] and delete-temp-file.cgi [5] and mark this bug WontFix afterwards? [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win.py?q=perl+wT&l=128 [2] https://perldoc.perl.org/File/Spec.html#tmpdir [3] https://perldoc.perl.org/File/Spec/Win32.html#tmpdir [4] https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi?q=unpopulated+environment&l=79 [5] https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/delete-temp-file.cgi?q=unpopulated+environment&l=53
,
Nov 27 2017
I seem to remember that even reading directly from %ENV I did not see any useful values -- in fact I believe my CGI Perl code does that in its current checked-in form, and in fact it's not seeing non-empty values in any of TEMP, TMPDIR or TMP - see https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi?q=CSIDL_LOCAL_APPDATA+file:%5C.cgi&sq=package:chromium&dr=C&l=92 - which is the only condition under which it falls back to Win32::GetFolderPath(Win32::CSIDL_LOCAL_APPDATA()); However I will try again and report back.
,
Nov 27 2017
(And semi0tangentially: I think TMP, TEMP and TMPDIR should not be considered untrustworthy in this CGI context, given that those aren't environment variables writable by manipulating HTTP request headers or other HTTP client-controlled request parameters unless there is a serious bug in the web server's CGI implementation.)
,
Nov 28 2017
I've modified the CGI to record TMP, TEMP, and TMPDIR to stderr. On Linux, local third_party/WebKit/Tools/Scripts/run-webkit-tests does not show any of those variables set inside the CGI, and Perl's tmpdir() defaults to /tmp:
[Mon Nov 27 21:26:49.721084 2017] [cgi:error] [pid 9333] [client 127.0.0.1:36768] AH01215: tmpdir() => /tmp: /usr/local/google/home/bsittler/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form.html
[Mon Nov 27 21:26:49.721131 2017] [cgi:error] [pid 9333] [client 127.0.0.1:36768] AH01215: $ENV{TMP} => (unset): /usr/local/google/home/bsittler/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form.html
[Mon Nov 27 21:26:49.721143 2017] [cgi:error] [pid 9333] [client 127.0.0.1:36768] AH01215: $ENV{TEMP} => (unset): /usr/local/google/home/bsittler/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form.html
[Mon Nov 27 21:26:49.721166 2017] [cgi:error] [pid 9333] [client 127.0.0.1:36768] AH01215: $ENV{TMPDIR} => (unset): /usr/local/google/home/bsittler/chromium/src/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form.html
This is regardless of whether I set+export those variables in the shell from which I run the tests.
So, even in local Linux test runs this is not working. Running the CGI "by hand" outside of the test environment, it does in fact see those environment variables when they are exported by the parent process.
I'll upload the change and try it on the bots next.
,
Nov 28 2017
(uploaded as https://crrev.com/c/792610 and started dry run)
,
Nov 28 2017
Linux trybot shows no TMP, TEMP, or TMPDIR: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/596406 https://isolateserver.appspot.com/browse?namespace=default-gzip&hash=2cddd1057b68e76c7f1b72579c230d9d7724e61f https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=f412ff31a8d29c6b5fa443c29e2017217a0cf49b&as=error_log.txt [Mon Nov 27 18:59:32.141576 2017] [cgi:error] [pid 1877] [client 127.0.0.1:33045] AH01215: tmpdir() => /tmp, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 18:59:32.141630 2017] [cgi:error] [pid 1877] [client 127.0.0.1:33045] AH01215: $ENV{TMP} => (unset), referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 18:59:32.141641 2017] [cgi:error] [pid 1877] [client 127.0.0.1:33045] AH01215: $ENV{TEMP} => (unset), referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 18:59:32.141651 2017] [cgi:error] [pid 1877] [client 127.0.0.1:33045] AH01215: $ENV{TMPDIR} => (unset), referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html
,
Nov 28 2017
Mac has the same behavior: https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/598704 https://isolateserver.appspot.com/browse?namespace=default-gzip&hash=a7c509fe5163adf0b6ffa71c710c97de38b57903 https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=6378fec353cc0546e945bf082e23110ffe4b0801&as=error_log.txt [Mon Nov 27 18:49:22.022953 2017] [cgi:error] [pid 3080] [client 127.0.0.1:59447] AH01215: tmpdir() => /tmp: /b/s/w/ir/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-x-user-defined-part1.html [Mon Nov 27 18:49:22.023114 2017] [cgi:error] [pid 3080] [client 127.0.0.1:59447] AH01215: $ENV{TMP} => (unset): /b/s/w/ir/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-x-user-defined-part1.html [Mon Nov 27 18:49:22.023152 2017] [cgi:error] [pid 3080] [client 127.0.0.1:59447] AH01215: $ENV{TEMP} => (unset): /b/s/w/ir/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-x-user-defined-part1.html [Mon Nov 27 18:49:22.023172 2017] [cgi:error] [pid 3080] [client 127.0.0.1:59447] AH01215: $ENV{TMPDIR} => (unset): /b/s/w/ir/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-x-user-defined-part1.html
,
Nov 28 2017
... and so of course does Windows, but with the added awesomeness that tmpdir() defaults to '\' there! https://build.chromium.org/p/tryserver.chromium.win/builders/win7_chromium_rel_ng/builds/53092 https://isolateserver.appspot.com/browse?namespace=default-gzip&hash=6415746d5a65030acf7ac523ec941711a76e46dc https://isolateserver.appspot.com/browse?namespace=default-gzip&digest=3ad42b2cbdab1880d8bb6d6760d3376b36a41bb2&as=layout-test-results%5Cerror_log.txt [Mon Nov 27 19:05:42 2017] [error] [client 127.0.0.1] tmpdir() => \\, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 19:05:42 2017] [error] [client 127.0.0.1] $ENV{TMP} => (unset), referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 19:05:42 2017] [error] [client 127.0.0.1] $ENV{TEMP} => (unset), referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 19:05:42 2017] [error] [client 127.0.0.1] $ENV{TMPDIR} => (unset), referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html [Mon Nov 27 19:05:42 2017] [error] [client 127.0.0.1] GetFolderPath(CSIDL_LOCAL_APPDATA()) => C:\\Users\\chrome-bot\\AppData\\Local, referer: http://127.0.0.1:8000/fileapi/send-dragged-file-form-windows-1252-part1.html
,
Nov 28 2017
So, apologies for misinformation in #2 - I thought the variables were being set on Mac and Linux, but actually they weren't - it's just that tmpdir() has a slightly saner default on those platforms.
,
Nov 28 2017
Re #7 and #4 - at least in the non-Win32 case, TMPDIR from https://cs.chromium.org/chromium/infra/luci/client/run_isolated.py?l=301 is not in the passed-through list in https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py?rcl=7f21c4b000691cd2580a7e5841a94ce3cafde9ae&l=1101
,
Nov 28 2017
It seems our Apaches run without PassEnv TMPDIR / PassEnv TEMP Also, the Python scripts' attempts to override environment variables using os.execvpe don't actually seem to work, at least in local Linux testing
,
Nov 28 2017
(And just adding PassEnv without changing the Python causes fatal errors because PassEnv croaks when the named variable does not exist!)
,
Nov 28 2017
... and we of course can't just add PassEnv because our windows Apache installation is missing mod_env - it is apparently present neither as a built-in module nor as a loadable module.
,
Nov 28 2017
How hard would it be to add mod_env to our Windows Apache installation? Or is it already there but with an unexpected module file name? Its absence prevents CGIs from honoring the per-test-run temporary directory setting. http://httpd.apache.org/docs/2.4/mod/mod_env.html#passenv
,
Nov 28 2017
It should be fairly easy to add.
,
Nov 29 2017
,
Nov 29 2017
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a8306aec2dd3732ad0e6976b40ca19e2e34c6123 commit a8306aec2dd3732ad0e6976b40ca19e2e34c6123 Author: Benjamin C. Wiley Sittler <bsittler@chromium.org> Date: Thu Nov 30 05:00:23 2017 apache: pass $TMPDIR (win32: also %TEMP% and %TMP%) through to CGIs Bug: 786152 Change-Id: Ib0e683a0fc93e485f16b4a99278e4e66fc0f6d1a Reviewed-on: https://chromium-review.googlesource.com/792610 Commit-Queue: Benjamin Wiley Sittler <bsittler@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#520446} [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/delete-temp-file.cgi [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/LayoutTests/http/tests/fileapi/resources/write-temp-file.cgi [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/apache2-httpd-2.2.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/apache2-httpd-2.4.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/arch-httpd-2.4.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/cygwin-httpd.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/debian-httpd-2.2.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/debian-httpd-2.4.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/redhat-httpd-2.2.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/redhat-httpd-2.4.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/apache_config/win-httpd.conf [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win.py [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/apache-win32/README.chromium [add] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/apache-win32/modules/mod_env.so.sha1 [modify] https://crrev.com/a8306aec2dd3732ad0e6976b40ca19e2e34c6123/third_party/apache-win32/remove_files_not_needed_for_chromium.sh
,
Nov 30 2017
The change might have caused bug 789820, we should try to find out why, and probably revert and then try to reland.
,
Nov 30 2017
#26: I believe that's a separate (non-Chromium) issue and actually has existed for some time with the affected Linux distributions due to failure of the system package manager to delete /etc/apache2/mods-available/php7.0.load after upgrading from PHP 7.0 to PHP 7.1.
,
Nov 30 2017
Summarizing review comments and private chat should anyone ever need to look into this again: the new Apache configs forward the TMPDIR / TEMP variables from the environment used to launch Apache to CGI scripts (at least Perl), and the LayoutTest scripts set up TMPDIR / TEMP as appropriate when launching Apache. This should fix the issue for all configurations where running LayoutTests is actively supported. There is a possibility that odd environments (like Cygwin) would use a relative path that is not correctly turned into an absolute path by the LayoutTest launch scripts. Since we don't actively support Cygwin, we haven't looked into that. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by pwnall@chromium.org
, Nov 16 2017