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 Thu, Aug 12, 2010 at 08:42:43PM +0200, Corinna Vinschen wrote:
>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 (

Shouldn't this be system_printf?  Otherwise it won't show up in an strace.
And, why not just use %E rather than calling GetLastError() directly?

>+"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;

A small nit but rather than put the knowledge for what these error codes
mean in spawn_guts shouldn't it be coming a from cwd.strerror() or
something like that?

cgf


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