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

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

On Aug 10 21:53, John Carey wrote:
> On Aug 10 13:44 +0200 Corinna Vinschen wrote:
> >   chdir ("/");
> >   h = CreateFile ("foobar", ...);
> >   if (h == INVALID_HANDLE_VALUE) printf ("%lu\n", GetLastError());
> 
> Thanks for the test case for the CreateFile() problem; I used it to
> create the following test, in which Windows 7 CreateFile() fails with
> ERROR_INVALID_HANDLE even when using a stock Cygwin 1.7.5 DLL:
> [...]

Thanks for the test case.

> The PEB lock in cwdstuff::set() can only prevent the race if other
> threads always hold the PEB lock while invoking system calls that
> allocate handles.  CreateFile() is pretty big, so I might have missed
> it, but I did not see CreateFile() holding the PEB lock while actually
> creating the new file handle.

No, it doesn't have to.  It only needs the PEB lock when accessing
the cwd in the PEB.  But ...

>   Instead, after actually creating the
> new file handle, it called _RtlReleaseRelativeName@4 on
> a reference-counted object that holds a copy of the current
> directory handle (see below for more about such reference-counted
> current directory objects).

... if that's correct it seems that the cwd from the PEB isn't even used
in Vista++.  That explains why just exchanging the handle value in
cwdstuff::set() only works fine in 2K3 and earlier systems.  Bummer.
I assume the idea was to speed up CreateFile by getting rid of a lock
per call.

> Now, where is Windows stuffing the extra copy of the directory handle?
> In those reference-counted heap-allocated directory objects.  Stepping
> through the machine code under Windows 7 I see SetCurrentDirectory()
> updating a global pointer ("ntdll!RtlpCurDirRef") to one such object,
> under the protection of a critical section ("ntdll!FastPebLock").
> Despite mentioning "Peb" in the name, neither global's address is
> computed using "FS:".  The global pointer points to a heap-allocated
> block that starts with a reference count followed by a copy of the
> directory handle value, and apparently includes other data as well.
> SetCurrentDirectory() swaps in a new such block, and decrements the
> counter on the old block, and if that reaches 0, closes the handle.
> Anyway, this block appears to be where the old current directory
> handle value persists past the changes made by cwdstuff::set().

...and that handle is used in subsequent calls and potentially is a
handle to another object in the meantime.

I do basically agree with cgf that it's your own problem if you use
Win32 calls in your Cygwin app.  OTOH, I see that this can trip up
potential handle problems in the DLL itself.

Unfortunately that means there's no Win32-safe way to set a new
directory handle as cwd in Vista and later anymore.  Since there's no
official API to set the cwd using a directory handle, there's no way to
set the Win32 cwd to a directory with restricted permissions.
This *is* frustrating.

I'll look into another solution.  Probably we will have to call
SetCurrentDirectory again and ignore any error.  I don't accept the
aforementioned restriction for POSIX calls.


Corinna

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

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