This is the mail archive of the cygwin-developers mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Aug 12 12:44, Christopher Faylor wrote:
> On Thu, Aug 12, 2010 at 06:26:21PM +0200, Corinna Vinschen wrote:
> >  SetCurrentDirectory ("//?/PIPE/");
> >  CreateFile ("../C:/foo", ...);
> >
> >works.  Go figure!
> 
> I meant to mention that //?/NUL/ "works" too except that CreateFile
> always succeeds.  I was close to retracting my previous position and go
> with that just because it really deliciously highlights the WJM
> principle.
> 
> Cygwin user:
> 
> "Where did my file go?  CreateFile succeeded!"
> 
> Response:
> 
> "Mwahahaha!"

*smirk*
I would go with that as well, but I used the PIPE fs for now.

> Btw, I can now run my VirtualBox stuff again.  Just had to rebuild
> linux, reboot, download the latest nvidia drivers, and rebuild
> virtualbox.
> 
> Simple.

Yep, easy.

Here's what I came up with.  I also improved the error message in
spawn_guts:

  [~]# cmd
  Microsoft Windows [Version 6.1.7600]
  Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

  C:\cygwin\home\corinna>exit
  exit
  [~]# cd tmp/1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890/
  [...]# cmd
  Error: Current working directory has a path longer than allowed for a
  Win32 working directory.
  Can't start native Windows application from here.

  /cygdrive/c/Windows/system32/cmd: File name too long.
  [...]# cd /proc
  [/proc]# cmd
  Error: Current working directory is a virtual Cygwin directory.
  Can't start native Windows application from here.

  cmd: Command not found.
  [/proc]# cd /cygdrive/c/System\ Volume\ Information/
  [/cygdrive/c/System Volume Information]# cmd
  Error: Current working directory has restricted permissions which render it
  inaccessible as Win32 working directory.
  Can't start native Windows application from here.

  /cygdrive/c/Windows/system32/cmd: Permission denied.

Patch below.  Does that look ok?  Would you mind to give it a test
yourselves (not only cgf, if possible)?

Andy, while testing I got a strange error dialog from mintty once, when
just clicking into the mintty window.  "system can not find path" or
something like that.  Afterwards, mintty simply exited.  Is it possible
that this is related to this patch?  Unfortunately I couldn't reproduce
it.


Thanks,
Corinna


	* cygheap.h (class cwdstuff): Make drive_length private.
	Add "error" member.
	(cwdstuff::get_error): New inline method.
	(cwdstuff::set): Change first parameter to pointer to path_conv.
	Drop unused third parameter.
	* path.cc (chdir): Drop doit.  Align call to cwdstuff::set to
	new arguments.
	(cwdstuff::init): Drop third parameter in call to cwdstuff::set.
	(cwdstuff::set): Partially rewrite.  Drop "doit" since it's not
	used anymore.  Always create new handle to CWD if not in a virtual
	path.  Check for accessibility to set correct error code.  Drop
	Vista workaround.  Never write back into PEB.  Set Win32 CWD to
	\\?\PIPE\ on init.  Simplify creation of win32 path.  Set new
	error member to a meaningful value.
	* spawn.cc (spawn_guts): Create more meaningful error message when
	not being able to start native Win32 app due to CWD restrictions.
	When starting native Win32 app, lock cwd and use in calls to
	CreateProcessW/CreateProcessAsUserW.


Index: cygheap.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/cygheap.h,v
retrieving revision 1.145
diff -u -p -r1.145 cygheap.h
--- cygheap.h	9 Aug 2010 16:53:35 -0000	1.145
+++ cygheap.h	12 Aug 2010 18:39:08 -0000
@@ -211,9 +211,14 @@ class cwdstuff
 private:
   char *posix;
   HANDLE dir;
+  DWORD drive_length;
+  int error;		/* This contains an errno number which corresponds
+			   to the problem with this path when trying to start
+			   a native Win32 application.  See cwdstuff::set for
+			   how it gets set.  See spawn_guts for how it's
+			   evaluated. */
 public:
   UNICODE_STRING win32;
-  DWORD drive_length;
   static muto cwd_lock;
   const char *get_posix () const { return posix; };
   void reset_posix (wchar_t *);
@@ -226,8 +231,9 @@ public:
     cwd_lock.release ();
     return ret;
   }
+  int get_error () const { return error; }
   void init ();
-  int set (PUNICODE_STRING, const char *, bool);
+  int set (path_conv *, const char *);
 };
 
 #ifdef DEBUGGING
Index: path.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.599
diff -u -p -r1.599 path.cc
--- path.cc	4 Aug 2010 11:25:13 -0000	1.599
+++ path.cc	12 Aug 2010 18:39:08 -0000
@@ -2729,7 +2729,6 @@ chdir (const char *in_dir)
     }
 
   int res = -1;
-  bool doit = false;
   const char *posix_cwd = NULL;
   int devn = path.get_devn ();
   if (!path.exists ())
@@ -2746,7 +2745,6 @@ chdir (const char *in_dir)
       if (!isdrive(path.normalized_path))
 	posix_cwd = path.normalized_path;
       res = 0;
-      doit = true;
     }
   else
    {
@@ -2755,7 +2753,7 @@ chdir (const char *in_dir)
    }
 
   if (!res)
-    res = cygheap->cwd.set (path.get_nt_native_path (), posix_cwd, doit);
+    res = cygheap->cwd.set (&path, posix_cwd);
 
   /* Note that we're accessing cwd.posix without a lock here.  I didn't think
      it was worth locking just for strace. */
@@ -3266,116 +3264,113 @@ void
 cwdstuff::init ()
 {
   cwd_lock.init ("cwd_lock");
+
   /* Initially re-open the cwd to allow POSIX semantics. */
-  set (NULL, NULL, true);
+  set (NULL, NULL);
 }
 
 /* Chdir and fill out the elements of a cwdstuff struct. */
 int
-cwdstuff::set (PUNICODE_STRING nat_cwd, const char *posix_cwd, bool doit)
+cwdstuff::set (path_conv *nat_cwd, const char *posix_cwd)
 {
   int res = 0;
   UNICODE_STRING upath;
   size_t len = 0;
+  bool virtual_path = false;
+  bool unc_path = false;
+  bool inaccessible_path = false;
 
   cwd_lock.acquire ();
 
   if (nat_cwd)
     {
-      upath = *nat_cwd;
-      if (upath.Buffer[0] == L'/') /* Virtual path.  Never use in PEB. */
-	doit = false;
+
+      upath = *nat_cwd->get_nt_native_path ();
+      if (nat_cwd->isspecial ())
+	virtual_path = true;
       else
 	{
 	  len = upath.Length / sizeof (WCHAR) - 4;
 	  if (RtlEqualUnicodePathPrefix (&upath, &ro_u_uncp, TRUE))
-	    len -= 2;
+	    {
+	      len -= 2;
+	      unc_path = true;
+	    }
 	}
     }
 
-  if (doit)
+  /* Here are the problems with using SetCurrentDirectory:
+     
+     - SetCurrentDirectory only supports paths of up to MAX_PATH - 1 chars,
+       including a trailing backslash.  That's an absolute restriction, even
+       in the UNICODE API.
+
+     - SetCurrentDirectory fails for directories with strict
+       permissions even for processes with the SE_BACKUP_NAME
+       privilege enabled.  The reason is apparently that
+       SetCurrentDirectory calls NtOpenFile without the
+       FILE_OPEN_FOR_BACKUP_INTENT flag set.
+
+     - SetCurrentDirectory does not support case-sensitivity.
+
+     - Unlinking a cwd fails because SetCurrentDirectory seems to
+       open directories so that deleting the directory is disallowed.
+
+     - SetCurrentDirectory can naturally not work on virtual Cygwin paths
+       like /proc or /cygdrive.
+
+     Therefore, we do without SetCurrentDirectory and handle the CWD all
+     by ourselves.  To avoid surprising behaviour in the Win32 API which
+     would stem from the fact that the Win32 CWD is different from the
+     POSIX CWD, we move the Win32 CWD to an invalid directory in which
+     typical relative Win32 path handling fails.  */
+
+  /* Open a directory handle with FILE_OPEN_FOR_BACKUP_INTENT and with
+     all sharing flags set.  The handle is right now used in execptions.h
+     only, but that might change in future. */
+  if (!virtual_path)
     {
-      /* We utilize the user parameter block.  The directory is
-	 stored manually there.  Why the hassle?
-
-	 - SetCurrentDirectory fails for directories with strict
-	   permissions even for processes with the SE_BACKUP_NAME
-	   privilege enabled.  The reason is apparently that
-	   SetCurrentDirectory calls NtOpenFile without the
-	   FILE_OPEN_FOR_BACKUP_INTENT flag set.
-
-	 - Unlinking a cwd fails because SetCurrentDirectory seems to
-	   open directories so that deleting the directory is disallowed.
-	   The below code opens with *all* sharing flags set. */
       HANDLE h;
       NTSTATUS status;
       IO_STATUS_BLOCK io;
       OBJECT_ATTRIBUTES attr;
-      PHANDLE phdl;
 
-      RtlAcquirePebLock ();
-      phdl = &get_user_proc_parms ()->CurrentDirectoryHandle;
       if (!nat_cwd) /* On init, just reopen CWD with desired access flags. */
-	RtlInitUnicodeString (&upath, L"");
-      /* This is for Win32 apps only.  No case sensitivity here... */
-      InitializeObjectAttributes (&attr, &upath,
-				  OBJ_CASE_INSENSITIVE | OBJ_INHERIT,
-				  nat_cwd ? NULL : *phdl, NULL);
+	{
+	  RtlInitUnicodeString (&upath, L"");
+	  RtlAcquirePebLock ();
+	  PHANDLE phdl = &get_user_proc_parms ()->CurrentDirectoryHandle;
+	  InitializeObjectAttributes (&attr, &upath,
+				      OBJ_CASE_INSENSITIVE | OBJ_INHERIT,
+				      *phdl, NULL);
+	  RtlReleasePebLock ();
+	}
+      else
+	InitializeObjectAttributes (&attr, &upath,
+				    nat_cwd->objcaseinsensitive ()
+				    | OBJ_INHERIT, NULL, NULL);
+      /* First try without FILE_OPEN_FOR_BACKUP_INTENT, to find out if the
+         directory is valid for Win32 apps. */
       status = NtOpenFile (&h, SYNCHRONIZE | FILE_TRAVERSE, &attr, &io,
 			   FILE_SHARE_VALID_FLAGS,
 			   FILE_DIRECTORY_FILE
-			   | FILE_SYNCHRONOUS_IO_NONALERT
-			   | FILE_OPEN_FOR_BACKUP_INTENT);
+			   | FILE_SYNCHRONOUS_IO_NONALERT);
+      if (status == STATUS_ACCESS_DENIED)
+      	{
+	  status = NtOpenFile (&h, SYNCHRONIZE | FILE_TRAVERSE, &attr, &io,
+			       FILE_SHARE_VALID_FLAGS,
+			       FILE_DIRECTORY_FILE
+			       | FILE_SYNCHRONOUS_IO_NONALERT
+			       | FILE_OPEN_FOR_BACKUP_INTENT);
+	  inaccessible_path = true;
+	}
       if (!NT_SUCCESS (status))
 	{
-	  RtlReleasePebLock ();
 	  __seterrno_from_nt_status (status);
 	  res = -1;
 	  goto out;
 	}
-      /* Workaround a problem in Vista/Longhorn which fails in subsequent
-	 calls to CreateFile with ERROR_INVALID_HANDLE if the handle in
-	 CurrentDirectoryHandle changes without calling SetCurrentDirectory,
-	 and the filename given to CreateFile is a relative path.  It looks
-	 like Vista stores a copy of the CWD handle in some other undocumented
-	 place.  The NtClose/DuplicateHandle reuses the original handle for
-	 the copy of the new handle and the next CreateFile works.
-	 Note that this is not thread-safe (yet?) */
-      NtClose (*phdl);
-      if (DuplicateHandle (GetCurrentProcess (), h, GetCurrentProcess (), phdl,
-			   0, TRUE, DUPLICATE_SAME_ACCESS))
-	NtClose (h);
-      else
-	*phdl = h;
-      dir = *phdl;
-
-      /* No need to set path on init. */
-      if (nat_cwd
-	  /* TODO:
-	     Check the length of the new CWD.  Windows can only handle
-	     CWDs of up to MAX_PATH length, including a trailing backslash.
-	     If the path is longer, it's not an error condition for Cygwin,
-	     so we don't fail.  Windows on the other hand has a problem now.
-	     For now, we just don't store the path in the PEB and proceed as
-	     usual. */
-	  && len <= MAX_PATH - (nat_cwd->Buffer[len - 1] == L'\\' ? 1 : 2))
-	{
-	  /* Convert to a Win32 path. */
-	  upath.Buffer += upath.Length / sizeof (WCHAR) - len;
-	  if (upath.Buffer[1] == L'\\') /* UNC path */
-	    upath.Buffer[0] = L'\\';
-	  upath.Length = len * sizeof (WCHAR);
-	  /* Append backslash if necessary. */
-	  if (upath.Buffer[len - 1] != L'\\')
-	    {
-	      upath.Buffer[len] = L'\\';
-	      upath.Length += sizeof (WCHAR);
-	    }
-	  RtlCopyUnicodeString (&get_user_proc_parms ()->CurrentDirectoryName,
-				&upath);
-	}
-
-      RtlReleasePebLock ();
+      dir = h;
     }
 
   if (nat_cwd || !win32.Buffer)
@@ -3395,32 +3390,36 @@ cwdstuff::set (PUNICODE_STRING nat_cwd, 
 	  RtlReleasePebLock ();
 
 	  PWSTR eoBuffer = win32.Buffer + (win32.Length / sizeof (WCHAR));
-	  /* Remove trailing slash if one exists.  FIXME: Is there a better way to
-	     do this? */
+	  /* Remove trailing slash if one exists. */
 	  if ((eoBuffer - win32.Buffer) > 3 && eoBuffer[-1] == L'\\')
 	    win32.Length -= sizeof (WCHAR);
+	  if (eoBuffer[0] == L'\\')
+	    unc_path = true;
 
 	  posix_cwd = NULL;
+
+	  /* When inited move the actual Win32 CWD out of the way, as explained
+	     above.  Surprisingly, the PIPE filesystem seems to be usable as CWD
+	     on all Windows systems. */
+	  if (!SetCurrentDirectoryW (L"\\\\?\\PIPE\\"))
+	    small_printf (
+"WARNING: Couldn't set Win32 CWD to //?/PIPE (error %lu).  This will\n"
+"probably not affect normal POSIX path operations.  However, please report\n"
+"this problem to the mailing list mailto:cygwin@cygwin.com.  Thank you.",
+			  GetLastError ());
 	}
       else
 	{
-	  bool unc = false;
-
-	  if (upath.Buffer[0] == L'/') /* Virtual path, don't mangle. */
+	  if (virtual_path) /* don't mangle virtual path. */
 	    ;
-	  else if (!doit)
+	  else
 	    {
 	      /* Convert to a Win32 path. */
 	      upath.Buffer += upath.Length / sizeof (WCHAR) - len;
-	      if (upath.Buffer[1] == L'\\') /* UNC path */
-		unc = true;
 	      upath.Length = len * sizeof (WCHAR);
-	    }
-	  else
-	    {
+
 	      PWSTR eoBuffer = upath.Buffer + (upath.Length / sizeof (WCHAR));
-	      /* Remove trailing slash if one exists.  FIXME: Is there a better way to
-		 do this? */
+	      /* Remove trailing slash if one exists. */
 	      if ((eoBuffer - upath.Buffer) > 3 && eoBuffer[-1] == L'\\')
 		upath.Length -= sizeof (WCHAR);
 	    }
@@ -3429,27 +3428,36 @@ cwdstuff::set (PUNICODE_STRING nat_cwd, 
 							      upath.Length + 2),
 				     upath.Length + 2);
 	  RtlCopyUnicodeString (&win32, &upath);
-	  if (unc)
+	  if (unc_path)
 	    win32.Buffer[0] = L'\\';
 	}
       /* Make sure it's NUL-terminated. */
       win32.Buffer[win32.Length / sizeof (WCHAR)] = L'\0';
-      if (!doit)			 /* Virtual path */
-	drive_length = 0;
-      else if (win32.Buffer[1] == L':')	 /* X: */
-	drive_length = 2;
-      else if (win32.Buffer[1] == L'\\') /* UNC path */
-	{
-	  PWCHAR ptr = wcschr (win32.Buffer + 2, L'\\');
-	  if (ptr)
-	    ptr = wcschr (ptr + 1, L'\\');
-	  if (ptr)
-	    drive_length = ptr - win32.Buffer;
+      error = 0;
+      if (virtual_path)
+	{
+	  drive_length = 0;
+	  error = ENOENT;
+	}
+      else
+	{
+	  if (!unc_path)
+	    drive_length = 2;
 	  else
-	    drive_length = win32.Length / sizeof (WCHAR);
+	    {
+	      PWCHAR ptr = wcschr (win32.Buffer + 2, L'\\');
+	      if (ptr)
+		ptr = wcschr (ptr + 1, L'\\');
+	      if (ptr)
+		drive_length = ptr - win32.Buffer;
+	      else
+		drive_length = win32.Length / sizeof (WCHAR);
+	    }
+	  if (inaccessible_path)
+	    error = EACCES;
+	  else if (win32.Length > (MAX_PATH - 2) * sizeof (WCHAR))
+	    error = ENAMETOOLONG;
 	}
-      else				 /* Shouldn't happen */
-	drive_length = 0;
 
       tmp_pathbuf tp;
       if (!posix_cwd)
Index: spawn.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/spawn.cc,v
retrieving revision 1.293
diff -u -p -r1.293 spawn.cc
--- spawn.cc	29 Jun 2010 10:37:23 -0000	1.293
+++ spawn.cc	12 Aug 2010 18:39:09 -0000
@@ -380,17 +380,30 @@ spawn_guts (const char *prog_arg, const 
   if (res)
     goto out;
 
-  if (!real_path.iscygexec ()
-      && (cygheap->cwd.drive_length == 0
-	  || cygheap->cwd.win32.Length >= MAX_PATH * sizeof (WCHAR)))
+  if (!real_path.iscygexec () && cygheap->cwd.get_error () != 0)
     {
-      small_printf ("Error: Current working directory is a %s.\n"
+      const char *reason;
+      switch (cygheap->cwd.get_error ())
+	{
+	case EACCES:
+	  reason = "has restricted permissions which render it\n"
+		   "inaccessible as Win32 working directory";
+	  break;
+	case ENOENT:
+	  reason = "is a virtual Cygwin directory";
+	  break;
+	case ENAMETOOLONG:
+	  reason = "has a path longer than allowed for a\n"
+		   "Win32 working directory";
+	  break;
+	default:
+	  reason = "is not accessible for some reason (shouldn't happen!)";
+	  break;
+	}
+      small_printf ("Error: Current working directory %s.\n"
 		    "Can't start native Windows application from here.\n\n",
-		    cygheap->cwd.drive_length == 0
-		    ? "virtual Cygwin directory"
-		    : "path longer than allowed for a\n"
-		      "Win32 working directory");
-      set_errno (ENAMETOOLONG);
+		    reason);
+      set_errno (cygheap->cwd.get_error ());
       res = -1;
       goto out;
     }
@@ -551,6 +564,14 @@ spawn_guts (const char *prog_arg, const 
 loop:
   cygheap->user.deimpersonate ();
 
+  PWCHAR cwd;
+  cwd = NULL;
+  if (!real_path.iscygexec())
+    {
+      cygheap->cwd.cwd_lock.acquire ();
+      cwd = cygheap->cwd.win32.Buffer;
+    }
+
   if (!cygheap->user.issetuid ()
       || (cygheap->user.saved_uid == cygheap->user.real_uid
 	  && cygheap->user.saved_gid == cygheap->user.real_gid
@@ -564,7 +585,7 @@ loop:
 			   TRUE,	  /* inherit handles from parent */
 			   c_flags,
 			   envblock,	  /* environment */
-			   NULL,
+			   cwd,
 			   &si,
 			   &pi);
     }
@@ -627,7 +648,7 @@ loop:
 			   TRUE,	  /* inherit handles from parent */
 			   c_flags,
 			   envblock,	  /* environment */
-			   NULL,
+			   cwd,
 			   &si,
 			   &pi);
       if (hwst)
@@ -642,6 +663,9 @@ loop:
 	}
     }
 
+  if (!real_path.iscygexec())
+    cygheap->cwd.cwd_lock.release ();
+    
   /* Restore impersonation. In case of _P_OVERLAY this isn't
      allowed since it would overwrite child data. */
   if (mode != _P_OVERLAY || !rc)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]