This is the mail archive of the
cygwin-developers
mailing list for the Cygwin project.
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