Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std: prevent CreateProcess() race on Windows #20650

Merged
merged 1 commit into from
Jan 7, 2015

Conversation

klutzy
Copy link
Contributor

@klutzy klutzy commented Jan 6, 2015

Believe or not, CreateProcess() is racy if several threads create
child processes: 0, 1, 2.

This caused some tests show crash dialogs during
make check-stage#-rpass.

More explanation:

On Windows, SetErrorMode() controls display of error dialogs: it
accepts new error mode and returns old error mode.
The error mode is process-global and automatically inherited to child
process when created.

MSYS2 bash shell internally sets it to not show error dialogs, therefore
make check-stage#-rpass should not show them either.

However, 1 says that CreateProcess() internally invokes
SetErrorMode() twice: at first it sets mode 0x8001 and saves
original mode, and at second it restores original mode.
So if two threads simultaneously call CreateProcess(), the first
thread sets error mode to 0x8001 then the second thread recognizes
that current error mode is 0x8001. Therefore, The second thread will
create process with wrong error mode.

This really occurs inside compiletest: it creates several processes on
each thread, so some run-pass tests are invoked with wrong error mode
therefore show crash dialog.

This commit adds StaticMutex for CreateProcess() call. This seems
to fix the "dialog annoyance" issue.

Believe or not, `CreateProcess()` is racy if several threads create
child processes: [0], [1], [2].

This caused some tests show crash dialogs during
`make check-stage#-rpass`.

More explanation:

On Windows, `SetErrorMode()` controls display of error dialogs: it
accepts new error mode and returns old error mode.
The error mode is process-global and automatically inherited to child
process when created.

MSYS2 bash shell internally sets it to not show error dialogs, therefore
`make check-stage#-rpass` should not show them either.

However, [1] says that `CreateProcess()` internally invokes
`SetErrorMode()` twice: at first it sets mode `0x8001` and saves
original mode, and at second it restores original mode.
So if two threads simultaneously call `CreateProcess()`, the first
thread sets error mode to `0x8001` then the second thread recognizes
that current error mode is `0x8001`. Therefore, The second thread will
create process with wrong error mode.

This really occurs inside `compiletest`: it creates several processes on
each thread, so some `run-pass` tests are invoked with wrong error mode
therefore show crash dialog.

This commit adds `StaticMutex` for `CreateProcess()` call. This seems
to fix the "dialog annoyance" issue.

[0]: http://support.microsoft.com/kb/315939
[1]: https://code.google.com/p/nativeclient/issues/detail?id=2968
[2]: https://ghc.haskell.org/trac/ghc/ticket/2650
@rust-highfive
Copy link
Collaborator

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

This is why we can't have nice things :(

@alexcrichton
Copy link
Member

Regardless, impressive investigation! Thanks so much for finding this!

@barosl
Copy link
Contributor

barosl commented Jan 6, 2015

I do hope this would fix some Windows-related parts of #19120. It will save us from tons of computational resource wastes. Great work, @klutzy!

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 7, 2015
Believe or not, `CreateProcess()` is racy if several threads create
child processes: [0], [1], [2].

This caused some tests show crash dialogs during
`make check-stage#-rpass`.

More explanation:

On Windows, `SetErrorMode()` controls display of error dialogs: it
accepts new error mode and returns old error mode.
The error mode is process-global and automatically inherited to child
process when created.

MSYS2 bash shell internally sets it to not show error dialogs, therefore
`make check-stage#-rpass` should not show them either.

However, [1] says that `CreateProcess()` internally invokes
`SetErrorMode()` twice: at first it sets mode `0x8001` and saves
original mode, and at second it restores original mode.
So if two threads simultaneously call `CreateProcess()`, the first
thread sets error mode to `0x8001` then the second thread recognizes
that current error mode is `0x8001`. Therefore, The second thread will
create process with wrong error mode.

This really occurs inside `compiletest`: it creates several processes on
each thread, so some `run-pass` tests are invoked with wrong error mode
therefore show crash dialog.

This commit adds `StaticMutex` for `CreateProcess()` call. This seems
to fix the "dialog annoyance" issue.

[0]: http://support.microsoft.com/kb/315939
[1]: https://code.google.com/p/nativeclient/issues/detail?id=2968
[2]: https://ghc.haskell.org/trac/ghc/ticket/2650
@klutzy
Copy link
Contributor Author

klutzy commented Jan 7, 2015

Hmm, I realized this may not be an ideal way in the long term. This patch addresses two issues:

  • race condition for CreateProcess(bInheritedHandles = TRUE). (We set bInheritedHandles = TRUE at std::sys::windows::process::spawn().)
  • race condition for process error mode.

For the first issue, lock has a drawback: non-Rust code still call CreateProcess() then the race continues.
There is better method without lock: explicitly pass inherited handles. http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx The APIs are from Vista though.
So, although lock fixes the issue, in the long term we may have to consider the explicit method.

For the second issue, it seems impossible to protect error mode without locking. However, @HNO3 found that GetLongPathName() also internally calls SetErrorMode() in a racy way! (tested on win7)
There seems no perfect way to preserve error mode on multi-thread environment.

Actually our goal is not to preserve error mode but just to avoid crash dialogs. We just may add Mutex at compiletest instead of std.

@nitric1
Copy link
Contributor

nitric1 commented Jan 7, 2015

(Stack trace of each SetErrorMode)
To be more specific (for leaving my investigation), SetErrorMode is called 4 times with arguments 0x8001, 0x8001, 0x8001 and original value during calling CreateProcess. Each argument means:

  1. setting "ignoring-message-box" mode to suppress "Cannot find" message box during CreateProcess
  2. setting "ignoring-message-box" mode to suppress same message box of GetFileAttribute used by GetLongPathName
  3. restoring original value (from 1)
  4. restoring original value (from before calling CreateProcess)

I think there are many similar problems related to using process error mode in multi-threaded environment.

Well, there's a brand new function SetThreadErrorMode which should be used in that APIs since Windows 7, but Microsoft might miss it…

@alexcrichton alexcrichton merged commit 68cb170 into rust-lang:master Jan 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants