Date: Tue, 10 Aug 2010 13:44:43 +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: <20100810114443.GA11766@calimero.vinschen.de>
Reply-To: cygwin AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
References: <3C031C390CBF1E4A8CE1F74DE7ECAF3A140684F0A2 AT MBX8 DOT EXCHPROD DOT USA DOT NET>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <3C031C390CBF1E4A8CE1F74DE7ECAF3A140684F0A2@MBX8.EXCHPROD.USA.NET>
User-Agent: Mutt/1.5.20 (2009-06-14)
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Precedence: bulk
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com

On Aug 10 00:19, John Carey wrote:
> After a call to SetCurrentDirectory(), I have seen occasional (< 5%)
> failures of CreatePipe() with code ERROR_INVALID_HANDLE.  I have also
> seen failure of Cygwin signal handling because the signal handling
> pipe has mysteriously closed.

Why on earth are you calling SetCurrentDirectory in a Cygwin application?

> I believe that both symptoms are due to this code in
> winsup/cygwin/path.cc, which as the comments state, is not
> thread-safe:
> 
>       /* 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);

you missed the

        else
	  *phdl = h;

> If another thread allocates a handle between the NtClose and the
> Duplicate, then the handle value is not preserved, and strange effects
> may result.

Note that phdl is a pointer to the handle storage within the PEB.  Since
the PEB is locked (RtlAcquirePebLock/RtlReleasePebLock) and Cygwin's
cwdstuff::set as well as the native SetCurrentDirectory both lock the
PEB before writing the CurDir content, there's no chance for a
concurrency issue between those functions.

Also note that the old content of *phdl is *always* overwritten, either
by the result of the DuplicateHandle call, or by the value of CreateFile.

> Presumably the issues mentioned in the source comment may
> also occur, but what I have seen myself is a subsequent call to
> SetCurrentDirectory() closing, not the current directory handle, but
> the handle that was allocated by the other thread.
> 
> Specifically, dll_crt0_1() calls sigproc_init(), then cwdstuff::set(),
> and finally wait_for_sigthread().  The function sigproc_init() creates
> the signal handling thread, which creates the signal handling pipe as
> one of its very first actions, then sets the event for which
> wait_for_sigthread() waits.  I think this scenario happens:
> 
>   1. main thread: cwdstuff::set(): NtClose().
> 
>   2. signal handling thread: CreatePipe() opens a handle to
>   '\Device\NamedPipe\' and stores it in a global variable (because
>   this is the first call to CreatePipe()).
> 
>   3. main thread: cwdstuff::set(): DuplicateHandle().
> 
> In this case, the current directory handle value has changed, which is
> not the intend of the NtClose-Duplicate sequence.

No, that's not intended.  However, the code just *tries* to preserve the
handle value, but it does not rely on it.  The NtClose is safe because
the handle is the actual CWD handle at the time of the call and the PEB
is locked.  The DuplicateHandle call just uses the phdl storage to store
the new handle value, but it *never* tests if the new handle value is
identical to the old one.  So, if all works well, it's the same handle
value as before.  If not, *shrug*.

> CreateFile to fail with ERROR_INVALID_HANDLE as mentioned in the
> source comments, but I have not seen that.  I think that the

I have.  You can easily test this as well on Vista and W7.  Just drop
the DuplicateHandle call and simply store h in *phdl.  Then create a
testcase:

  chdir ("/");
  h = CreateFile ("foobar", ...);
  if (h == INVALID_HANDLE_VALUE) printf ("%lu\n", GetLastError());

> CreatePipe() failures I have seen are triggered when
> SetCurrentDirectory() closes the handle to '\Device\NamedPipe\',
> thinking that it is the current directory handle.  After that,
> CreatePipe() will fail with ERROR_INVALID_HANDLE.

As mentioned above, calling SetCurrentDirectory in a Cygwin application
is not correct at all.  All relative path handling in Cygwin relies on
the fact that a Cygwin application calls the chdir function.  If you're
calling SetCurrentDirectory you're on your own.

And then again, note that the call to SetCurrentDirectory will not
overwrite the PEB value of the cwd handle until the cwdstuff::set
function has called RtlReleasePebLock.

> I think this other scenario also happens:
> 
>   1. signal handling thread: CreatePipe() opens a handle to
>   '\Device\NamedPipe\' (because it is the first call to CreatePipe()).
> 
>   2. main thread: cwdstuff::set(): NtClose().
> 
>   3. signal handling thread: CreatePipe() opens pipe handles.
> 
>   4. main thread: cwdstuff::set(): DuplicateHandle().
> 
> In this case it is Cygwin signal handling that is sabotaged by
> subsequent calls to SetCurrentDirectory(), because they close one of
> the pipe handles used for Cygwin signal handling.

This is pure speculation.  Please explain to me how DuplicateHandle,
which duplicates a *local* handle of a locally opened directory, which
is stored in the PEB's CurrentDirectoryHandle under RtlAcquirePebLock
conditions, should interfere with CreatePipe or, FWIW, SetCurrentDirectory.

The worst thing which could happen is that the handle in
PEB->CurrentDirectoryHandle has another value and a subsequent
CreateFile w/ a relative path fails (but not Cygwin's calls to
NtCreateFile).  I don't see any chance that one of the pipe handles is
suddenly stored in phdl == the address of the PEB's cwd handle.  And, if
DuplicateHandle fails, the local handle is stored in the PEB directly.
Still, all of that is done under RtlAcquirePebLock condition.

And then again, if CreatePipe() stores a global handle to the pipe
filesystem (I don't know, I never observed the CreatePipe call more
closely), it will probably store the handle in the PEB.  If it does,
it will use RtlAcquirePebLock/RtlReleasePebLock as well.

> Note that replacing calls to SetCurrentDirectory() with chdir() could
> actually make the problem worse, because it would trigger the
> thread-unsafe code more frequently.  The call to SetCurrentDirectory()
> is merely making it possible to detect the problem sooner.

Sorry, but I can't see how this should occur.  The worst possible
problem is that the handle is reused between NtClose and
DuplicateHandle.  There's no chance that a subsequent call to chdir or
SetCurrentDirectory would be able to close an unrelated handle.  Only
two handles are ever closed in this function, the original cwd in the
PEB, and/or the local handle h.

Am I missing something?


Corinna

--
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