Fix #15149 test, and allow it to run with noexec /tmp on POSIX systems. #21498

quantheory · 2015-01-22T05:37:08Z

While trying to experiment with changes for some other issues, I noticed that the test for #15149 was failing because I have /tmp mounted as noexec on my Linux box, and that test tries to run out of a temporary directory. This may not be the most common case, but it's not rare by any means, because executing from a world-writable directory is a security problem. (For this reason, some kernel options/mods such as grsecurity also can prevent this on Linux.) I instead copy the executable to a directory created in the build tree, following the example of the process-spawn-with-unicode-params test.

After I made that change, I noticed that I'd made a mistake, but the test was still passing, because the "parent" process was not actually checking the status of the "child" process, meaning that the assertion in the child could never cause the overall test to fail. (I don't know if this has always been the case, or if it has something to do with either Windows or a change in the semantics of spawn.) So I fixed the test so that it would fail correctly, then fixed my original mistake so that it would pass again.

The one big problem with this is that I haven't set up any machines of my own so that I can build on Windows, which is the platform this test was targeted at in the first place! That might take a while to address on my end. So I need someone else to check this on Windows.

…e child panics before passing.

rust-highfive · 2015-01-22T05:37:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

huonw · 2015-01-22T05:45:23Z

@bors: r+ 06d4

bors · 2015-01-22T14:44:29Z

⌛ Testing commit 06d4019 with merge ee7b199...

bors · 2015-01-22T16:13:32Z

💔 Test failed - auto-win-64-nopt-t

…ing.

quantheory · 2015-01-22T19:02:43Z

Trying again. I think the problem was another bug in the original, an inconsistent use of EXE_SUFFIX. If the test fails again, I added more debugging output to the log, so that it should be more obvious why it failed.

alexcrichton · 2015-01-23T00:45:02Z

src/test/run-pass/issue-15149.rs

+    let my_path = os::self_exe_name().unwrap();
+    let my_dir  = my_path.dir_path();
+
+    let child_dir = Path::new(my_dir.join("issue-15149-child"));


Could this add some random substring to the end of the directory as well? It's possible that this test is run a few times in parallel and it would help prevent conflicts

… directory's name, and remove the directory after a successful test.

quantheory · 2015-01-23T01:56:37Z

I added some simple randomness to the directory name, and the test removes the directory when the test passes (to avoid accumulating a bunch of different directories when the test is run multiple times). The directory is not removed on failure, but I think that that might actually be helpful if the test fails. (Plus, I was feeling too lazy to implement a new RAII type to remove the directory on panic.)

Note that process-spawn-with-unicode-params uses the same directory name every time as well, but it would need slightly more care. In that case the child asserts that it's running in a particular directory, so it needs to have access to the same path that the parent used.

alexcrichton · 2015-01-23T04:35:20Z

Ah the other test is fine for now, thanks for doing this!

alexcrichton · 2015-01-23T04:35:31Z

@bors: r+ 7f45dc9 rollup

While trying to experiment with changes for some other issues, I noticed that the test for rust-lang#15149 was failing because I have `/tmp` mounted as `noexec` on my Linux box, and that test tries to run out of a temporary directory. This may not be the most common case, but it's not rare by any means, because executing from a world-writable directory is a security problem. (For this reason, some kernel options/mods such as grsecurity also can prevent this on Linux.) I instead copy the executable to a directory created in the build tree, following the example of the `process-spawn-with-unicode-params` test. After I made that change, I noticed that I'd made a mistake, but the test was still passing, because the "parent" process was not actually checking the status of the "child" process, meaning that the assertion in the child could never cause the overall test to fail. (I don't know if this has always been the case, or if it has something to do with either Windows or a change in the semantics of `spawn`.) So I fixed the test so that it would fail correctly, then fixed my original mistake so that it would pass again. The one big problem with this is that I haven't set up any machines of my own so that I can build on Windows, which is the platform this test was targeted at in the first place! That might take a while to address on my end. So I need someone else to check this on Windows.

barosl · 2015-01-24T19:19:42Z

This caused a test failure in the rollup.

issue-15149.rs:50

Left: "C:\\bot\\slave\\auto-win-64-nopt-t\\build\\obj\\x86_64-pc-windows-gnu\\test\\run-pass\\issue-15149-child-2281511721\\mytest.exe"
Right: "mytest.exe"

barosl · 2015-01-24T19:47:51Z

More strangely. on my local test, it seems that Command::new() does not respect the child's PATH, which was the point of the original fix. "File not found" error occurs when I compile the test and run it, but if I add os::setenv("PATH", path.clone()); right before the invocation, the error is gone.

Even more strangely, after the successful execution, my assertion error differs from the one from the buildbot.

Left: "mytest"
Right: "mytest.exe"

Note that the left arm does not have the .exe suffix, as well as the full path.

My system is Windows 8.1, so maybe this is the reason?

It seems that the filename check is not reliable on Windows, and I even believe the logic can be removed entirely.

quantheory · 2015-01-24T23:01:45Z

@barosl: I believe that I can mostly explain this now.

Looking more closely at src/libstd/sys/windows/process.rs, I find that if a program is found in the child's path, the child is called using the full absolute path (with extension). This is why the regression test failed.

If the program is not found in the child's path (either because an absolute path was used, or because it's just not there), we call CreateProcessW with whatever the user specified as the program. A relative path that is in the parent's path but not the child's path will therefore be preserved in the child args, while every other case will cause args[0] to be an absolute path.

The one thing that I don't get is why in your local path, the program is not being found in the child's path. This shouldn't vary by Windows version because libstd is doing the search, not Windows. But perhaps you are running into a Path bug or compatibility issue?

barosl · 2015-01-25T00:51:52Z

While the above output was resulted from the direct compilation of the test code, I've just set up my Windows compilation environment for the first time, and run make check-stage2.

Oddly, the check process failed earlier because of the doctest test. Considering the output, I think that is due to PATH containing a interim space. (See ; path=Files\OpenVPN\bin below)

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: couldn't create file (OS Error 3: 지정된 경로를 찾을 수 없습니다.
; path=Files\OpenVPN\bin;C:\Rust\bin;C:\msys64\usr\bin\site_perl;C:\msys64\usr\bin\vendor_perl;C:\msys64\usr\bin\core_perl \C\msys64\home\barosl\dev\rust\x86_64-pc-windows-gnu\stage2\bin\rustc.exe --out-dir \C\msys64\home\barosl\dev\rust\x86_64-pc-windows-gnu\test\run-make\cannot-read-embedded-idents -L \C\msys64\home\barosl\dev\rust\x86_64-pc-windows-gnu\test\run-make\cannot-read-embedded-idents\broken.rs; mode=truncate; access=write)', C:\msys64\home\barosl\dev\rust\src\libcore\result.rs:742
make[1]: *** [dotest] Error 101

------        ---------------------------------------------

/home/barosl/dev/rust/mk/tests.mk:1026: 'x86_64-pc-windows-gnu/test/run-make/cannot-read-embedded-idents-2-T-x86_64-pc-windows-gnu-H-x86_64-pc-windows-gnu.ok' 타겟에 대한 명령이 실패했습니다
make: *** [x86_64-pc-windows-gnu/test/run-make/cannot-read-embedded-idents-2-T-x86_64-pc-windows-gnu-H-x86_64-pc-windows-gnu.ok] 오류 2

But even after removing the space from PATH, another test failure occurs:

thread '<main>' panicked at 'assertion failed: err.as_slice().contains("unknown start of token")', create_and_compile.rs:43
make[1]: *** [dotest] Error 101

------        ---------------------------------------------

/home/barosl/dev/rust/mk/tests.mk:1026: 'x86_64-pc-windows-gnu/test/run-make/cannot-read-embedded-idents-2-T-x86_64-pc-windows-gnu-H-x86_64-pc-windows-gnu.ok' 타겟에 대한 명령이 실패했습니다
make: *** [x86_64-pc-windows-gnu/test/run-make/cannot-read-embedded-idents-2-T-x86_64-pc-windows-gnu-H-x86_64-pc-windows-gnu.ok] 오류 2

So I believe my system is fundamentally flawed. I still consider this a bug of the build infrastructure, but it may not be related to this PR. So it would be OK to ignore my case for now.

- Successful merges: #21108, #21445, #21498, #21504, #21532, #21535, #21539, #21540, #21541, #21550, #21560, #21573, #21579 - Failed merges:

For issue 15149 test, don't execute from tmpfs, and wait to see if th…

06d4019

…e child panics before passing.

rust-highfive assigned huonw Jan 22, 2015

Attempt fix for assertion on Windows, and add extra output for debugg…

918dd34

…ing.

alexcrichton reviewed Jan 23, 2015
View reviewed changes

Add a random number string to the end of the issue-15149 test's child…

7f45dc9

… directory's name, and remove the directory after a successful test.

flaper87 mentioned this pull request Jan 23, 2015

Rollup of 10 pull requests #21556

Closed

flaper87 mentioned this pull request Jan 24, 2015

Rollup of 13 pull requests #21582

Merged

quantheory closed this Jan 24, 2015

quantheory reopened this Jan 24, 2015

bors added a commit that referenced this pull request Jan 25, 2015

Auto merge of #21582 - FlaPer87:rollup, r=brson

102ab57

- Successful merges: #21108, #21445, #21498, #21504, #21532, #21535, #21539, #21540, #21541, #21550, #21560, #21573, #21579 - Failed merges:

bors merged commit 7f45dc9 into rust-lang:master Jan 25, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #15149 test, and allow it to run with noexec /tmp on POSIX systems. #21498

Fix #15149 test, and allow it to run with noexec /tmp on POSIX systems. #21498

quantheory commented Jan 22, 2015

rust-highfive commented Jan 22, 2015

huonw commented Jan 22, 2015

bors commented Jan 22, 2015

bors commented Jan 22, 2015

quantheory commented Jan 22, 2015

alexcrichton Jan 23, 2015

quantheory commented Jan 23, 2015

alexcrichton commented Jan 23, 2015

alexcrichton commented Jan 23, 2015

barosl commented Jan 24, 2015

barosl commented Jan 24, 2015

quantheory commented Jan 24, 2015

barosl commented Jan 25, 2015

Fix #15149 test, and allow it to run with noexec /tmp on POSIX systems. #21498

Fix #15149 test, and allow it to run with noexec /tmp on POSIX systems. #21498

Conversation

quantheory commented Jan 22, 2015

rust-highfive commented Jan 22, 2015

huonw commented Jan 22, 2015

bors commented Jan 22, 2015

bors commented Jan 22, 2015

quantheory commented Jan 22, 2015

alexcrichton Jan 23, 2015

Choose a reason for hiding this comment

quantheory commented Jan 23, 2015

alexcrichton commented Jan 23, 2015

alexcrichton commented Jan 23, 2015

barosl commented Jan 24, 2015

barosl commented Jan 24, 2015

quantheory commented Jan 24, 2015

barosl commented Jan 25, 2015