critical section around run to workaroudn pipe inheritance issue in w…

…in32

src/cpp/core/system/CriticalSection.hpp

@@ -0,0 +1,76 @@
+/*
+ * CriticalSection.hpp
+ *
+ * Copyright (C) 2009-11 by RStudio, Inc.
+ *
+ * This program is licensed to you under the terms of version 3 of the
+ * GNU Affero General Public License. This program is distributed WITHOUT
+ * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT,
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the
+ * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details.
+ *
+ */
+
+#ifndef CORE_SYSTEM_CRITICAL_SECTION_HPP
+#define CORE_SYSTEM_CRITICAL_SECTION_HPP
+
+#ifdef _WIN32
+
+#include <windows.h>
+
+namespace core {
+namespace system {
+
+// critical section wrapper
+class CriticalSection
+{
+public:
+   CriticalSection()
+   {
+      ::InitializeCriticalSection(&criticalSection_);
+   }
+
+   class Scope
+   {
+   public:
+      explicit Scope(CriticalSection& cs)
+         : cs_(cs)
+      {
+         cs_.enter();
+      }
+
+      virtual ~Scope()
+      {
+         try
+         {
+            cs_.leave();
+         }
+         catch(...)
+         {
+         }
+      }
+
+   private:
+      CriticalSection& cs_;
+   };
+
+private:
+   void enter()
+   {
+      ::EnterCriticalSection(&criticalSection_);
+   }
+   void leave()
+   {
+      ::LeaveCriticalSection(&criticalSection_);
+   }
+
+   CRITICAL_SECTION criticalSection_;
+};
+
+
+} // namespace system
+} // namespace core
+
+#endif // _WIN32
+
+#endif // CORE_SYSTEM_CRITICAL_SECTION_HPP

src/cpp/core/system/Win32ChildProcess.cpp

@@ -17,20 +17,22 @@
 
 #include <boost/foreach.hpp>
 
+#include "CriticalSection.hpp"
+
 // TODO: test locating of executables
 
 // TODO: note on PeekNamedPipe blocking in multithreaded app with
 // other blocking call to ReadFile active
 
 // TODO: consider a peek, read, sleep loop to avoid global read block
 
-// TODO: kb article on race condition in creating processes
-
 namespace core {
 namespace system {
 
 namespace {
 
+
+
 // close a handle then set it to NULL (so we can call this function
 // repeatedly without failure or other side effects)
 Error closeHandle(HANDLE* pHandle, const ErrorLocation& location)
@@ -208,36 +210,44 @@ Error ChildProcess::terminate()
       return Success();
 }
 
-
 Error ChildProcess::run()
-{
-   SECURITY_ATTRIBUTES sa;
-   sa.nLength = sizeof(SECURITY_ATTRIBUTES);
-   sa.bInheritHandle = TRUE;
-   sa.lpSecurityDescriptor = NULL;
+{   
+   // NOTE: if the run method is called from multiple threads in single app
+   // concurrently then a race condition can cause handles to get incorrectly
+   // directed. the workaround suggested by microsoft is to wrap the process
+   // creation code in a critical section. see this article for details:
+   //   http://support.microsoft.com/kb/315939
+   static CriticalSection s_runCriticalSection;
+   CriticalSection::Scope csScope(s_runCriticalSection);
 
    // Standard input pipe
    HANDLE hStdInRead;
-   if (!::CreatePipe(&hStdInRead, &pImpl_->hStdInWrite, &sa, 0))
+   if (!::CreatePipe(&hStdInRead, &pImpl_->hStdInWrite, NULL, 0))
       return systemError(::GetLastError(), ERROR_LOCATION);
    CloseHandleOnExitScope closeStdIn(&hStdInRead, ERROR_LOCATION);
-   if (!::SetHandleInformation(pImpl_->hStdInWrite, HANDLE_FLAG_INHERIT, 0) )
+   if (!::SetHandleInformation(hStdInRead,
+                               HANDLE_FLAG_INHERIT,
+                               HANDLE_FLAG_INHERIT))
       return systemError(::GetLastError(), ERROR_LOCATION);
 
    // Standard output pipe
    HANDLE hStdOutWrite;
-   if (!::CreatePipe(&pImpl_->hStdOutRead, &hStdOutWrite, &sa, 0))
+   if (!::CreatePipe(&pImpl_->hStdOutRead, &hStdOutWrite, NULL, 0))
       return systemError(::GetLastError(), ERROR_LOCATION);
    CloseHandleOnExitScope closeStdOut(&hStdOutWrite, ERROR_LOCATION);
-   if (!::SetHandleInformation(pImpl_->hStdOutRead, HANDLE_FLAG_INHERIT, 0) )
+   if (!::SetHandleInformation(hStdOutWrite,
+                               HANDLE_FLAG_INHERIT,
+                               HANDLE_FLAG_INHERIT) )
       return systemError(::GetLastError(), ERROR_LOCATION);
 
    // Standard error pipe
    HANDLE hStdErrWrite;
-   if (!::CreatePipe(&pImpl_->hStdErrRead, &hStdErrWrite, &sa, 0))
+   if (!::CreatePipe(&pImpl_->hStdErrRead, &hStdErrWrite, NULL, 0))
       return systemError(::GetLastError(), ERROR_LOCATION);
    CloseHandleOnExitScope closeStdErr(&hStdErrWrite, ERROR_LOCATION);
-   if (!::SetHandleInformation(pImpl_->hStdErrRead, HANDLE_FLAG_INHERIT, 0) )
+   if (!::SetHandleInformation(hStdErrWrite,
+                               HANDLE_FLAG_INHERIT,
+                               HANDLE_FLAG_INHERIT) )
       return systemError(::GetLastError(), ERROR_LOCATION);
 
    // populate startup info