Use CWD for temp files, and fall back to /tmp only for upstream tests. #361
Conversation
|
I'm in agreement. LGTM. However, one comment about the implementation: strings.HasPrefix(currentDirectory, runtime.GOROOT())I believe that is brittle, specifically that it may result in false negatives. However, it shouldn't result in false positives, which is why the PR LGTM and can be merged as is. It may result in false negatives in:
Those situations are possible but should be rare, so if you're okay with it, we can merge as is, and if the false negatives result in visible problems in situations where it matters, then you can work on improving it. Otherwise it'll be fine as is. |
|
Thanks for the feedback, @shurcooL. Since, in the specific case of upstream Go tests, which this code is meant to address, we do a Chdir to runtime.GOROOT(), then read the input file names from disk, one would hope that this would address both the symlink and case normalization issues. Still, running one of these tests manually, with a symlink or mismatched case, could lead to a failure. But if someone is running these tests manually in such an environment, hopefully they'll have enough clues to work around the shortcoming... and/or fix the problem at that time, as you suggest. |
|
Yep, that sounds good to me. |
Use CWD for temp files, and fall back to /tmp only for upstream tests.
|
@flimzy As always, thanks for contributing! |
This is an alternative to #334 to fix #303. It uses the CWD for temp files, but falls back to using /tmp only for files found in the system GOROOT path, which should not be vulnerable to the security risks of running nodejs code in /tmp.
If there are ever other situations where we need to run tests in read-only directories, this will fail. I don't know if such cases are likely ever to exist, but if they do, then #334 is probably the preferred solution.