1.7.5: Occasional failure of CreatePipe or signal handing due to thread-unsafe code in cwdstuff::set

[Corinna Vinschen]

On Sep 14 10:12, Corinna Vinschen wrote:
> On Sep 13 19:47, John Carey wrote:
> > Anyway, it looks to me as if overwriting the path in an existing
> > VistaCwd instance would confuse RtlGetCurrentDirectory_U() and
> > probably other Win32 API functions.  (It may be that the same is
> > true for the handle member; I have not tried to find that out.)
> > Cygwin should probably allocate a new VistaCwd instance,
> > as does the Win32 SetCurrentDirectory() implementation.
> 
> Hmm, I'm still wondering.  In theory we don't have to replace the
> directory name.  We just have to InterlockedExchange the handle, since
> we only need to replace the original handle which does not allow
> sharing-for-delete with our own handle which does.
> 
> All other border cases (virtual dir, directory with restricted rights)
> can be rightfully handled by setting the Win32 CWD to \\?\PIPE\ again.
> 
> That's at least worth a try, isn't it?

I applied the below patch to Cygwin CVS and it appears to work nicely.
The only potential race I can think of is, if another thread of the same
Cygwin process calls SetCurrentDirectory.  I'm inclined to let this
situation slip through the cracks since SetCurrentDirectory will already
mess up a mixed-mode Cygwin process.

The "wincap.has_transactions ()" is just for testing.  If we can use
that code, I would replace is with something like
"wincap.has_messed_up_cwd_handling()" or so.


Corinna


Index: cygheap.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/cygheap.h,v
retrieving revision 1.146
diff -u -p -r1.146 cygheap.h
--- cygheap.h	13 Aug 2010 11:51:53 -0000	1.146
+++ cygheap.h	14 Sep 2010 16:10:45 -0000
@@ -217,6 +217,8 @@ private:
 			   a native Win32 application.  See cwdstuff::set for
 			   how it gets set.  See spawn_guts for how it's
 			   evaluated. */
+  void override_win32_cwd ();
+
 public:
   UNICODE_STRING win32;
   static muto cwd_lock;
Index: path.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.608
diff -u -p -r1.608 path.cc
--- path.cc	14 Sep 2010 14:10:39 -0000	1.608
+++ path.cc	14 Sep 2010 16:10:45 -0000
@@ -3363,6 +3363,29 @@ get_user_proc_parms ()
   return NtCurrentTeb ()->Peb->ProcessParameters;
 }
 
+struct VistaCwd {
+  volatile LONG ReferenceCount;
+  HANDLE DirectoryHandle;
+  ULONG OldDismountCount;
+  UNICODE_STRING Path;
+  wchar_t Buffer[MAX_PATH];
+};
+
+void
+cwdstuff::override_win32_cwd ()
+{
+  if (wincap.has_transactions ())
+    {
+      VistaCwd *v_cwd = (VistaCwd *)
+	((PBYTE) get_user_proc_parms ()->CurrentDirectoryName.Buffer
+	- __builtin_offsetof (VistaCwd, Buffer));
+      (void) InterlockedExchangePointer (&v_cwd->DirectoryHandle, dir);
+    }
+  HANDLE h = InterlockedExchangePointer
+	(&get_user_proc_parms ()->CurrentDirectoryHandle, dir);
+  NtClose (h);
+}
+
 /* Initialize cygcwd 'muto' for serializing access to cwd info. */
 void
 cwdstuff::init ()
@@ -3370,9 +3393,13 @@ cwdstuff::init ()
   cwd_lock.init ("cwd_lock");
 
   /* Cygwin processes inherit the cwd from their parent.  If the win32 path
-     buffer is not NULL, the cwd struct is already set up. */
-  if (win32.Buffer)
-    return;
+     buffer is not NULL, the cwd struct is already set up, and we only
+     have to override the Win32 CWD with ours. */
+  if (win32.Buffer && !error)
+    {
+      override_win32_cwd ();
+      return;
+    }
 
   /* Initially re-open the cwd to allow POSIX semantics. */
   set (NULL, NULL);
@@ -3570,13 +3597,12 @@ cwdstuff::set (path_conv *nat_cwd, const
     }
   /* Keep the Win32 CWD in sync.  Don't check for error, other than for
      strace output.  Try to keep overhead low. */
-  if (nat_cwd)
-    {
-      status = RtlSetCurrentDirectory_U (error ? &ro_u_pipedir : &win32);
-      if (!NT_SUCCESS (status))
-	debug_printf ("RtlSetCurrentDirectory_U(%S) failed, %p",
-		      error ? &ro_u_pipedir : &win32, status);
-    }
+  status = RtlSetCurrentDirectory_U (error ? &ro_u_pipedir : &win32);
+  if (!NT_SUCCESS (status))
+    debug_printf ("RtlSetCurrentDirectory_U(%S) failed, %p",
+		  error ? &ro_u_pipedir : &win32, status);
+  else if (!error)
+    override_win32_cwd ();
 
   /* Eventually, create POSIX path if it's not set on entry. */
   tmp_pathbuf tp;

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple