X-Recipient: archive-cygwin AT delorie DOT com X-Spam-Check-By: sourceware.org Date: Tue, 10 Aug 2010 13:44:43 +0200 From: Corinna Vinschen 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 List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , 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 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