X-Recipient: archive-cygwin AT delorie DOT com
X-Spam-Check-By: sourceware.org
Date: Tue, 14 Sep 2010 18:11:38 +0200
From: Corinna Vinschen <corinna-cygwin AT cygwin DOT com>
To: cygwin AT cygwin DOT com
Subject: Re: 1.7.5: Occasional failure of CreatePipe or signal handing due to 	thread-unsafe code in cwdstuff::set
Message-ID: <20100914161138.GG15121@calimero.vinschen.de>
Reply-To: cygwin AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
References: <AANLkTimpBCKzUioEarGiKMQbTjN+6s9iDYm_zzZ2Lxr9 AT mail DOT gmail DOT com> <20100812081151 DOT GT14202 AT calimero DOT vinschen DOT de> <3C031C390CBF1E4A8CE1F74DE7ECAF3A158EDA702A AT MBX8 DOT EXCHPROD DOT USA DOT NET> <20100903073740 DOT GA1749 AT calimero DOT vinschen DOT de> <3C031C390CBF1E4A8CE1F74DE7ECAF3A158EDA702D AT MBX8 DOT EXCHPROD DOT USA DOT NET> <20100904092626 DOT GE16534 AT calimero DOT vinschen DOT de> <3C031C390CBF1E4A8CE1F74DE7ECAF3A158EDA704C AT MBX8 DOT EXCHPROD DOT USA DOT NET> <20100911103010 DOT GM16534 AT calimero DOT vinschen DOT de> <3C031C390CBF1E4A8CE1F74DE7ECAF3A15945FDD78 AT MBX8 DOT EXCHPROD DOT USA DOT NET> <20100914081225 DOT GE16534 AT calimero DOT vinschen DOT de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20100914081225.GE16534@calimero.vinschen.de>
User-Agent: Mutt/1.5.20 (2009-06-14)
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
List-Id: <cygwin.cygwin.com>
List-Unsubscribe: <mailto:cygwin-unsubscribe-archive-cygwin=delorie DOT com AT cygwin DOT com>
List-Subscribe: <mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive: <http://sourceware.org/ml/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Delivered-To: mailing list cygwin AT cygwin DOT com

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