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 Sep 13 19:47, John Carey wrote:
> On Sep 11 03:30, Corinna Vinschen:
> > Given that, wouldn't it be potentially possible to override the content
> > of curCwd?  The problem is probably (just as in my old Cygwin code up to
> > 1.7.5) to make sure that this is thread safe.  Probably we would still
> > need CwdCS.
> 
> Unfortunately, it looks as if Win32 API functions guarantee to each
> other that they will not modify the path in a VistaCwd whose reference
> count is >= 2.  (Though it looks like they may update OldDismountCount
> under a CwdCS lock.)
> 
> Consider RtlGetCurrentDirectory_U().  It shares a (non-exported)
> subroutine with other Win32 API functions that acquires a counted
> reference to the current VistaCwd instance under the protection of
> a lock on CwdCS.  (This subroutine also does some kind of freshness
> check against DismountCount.)  But that subroutine releases its
> lock before returning, and RtlGetCurrentDirectory_U() then uses the
> pathname in the returned VistaCwd instance *without* holding a lock.
> Specifically, it copies that pathname to a memory buffer passed in by
> its own caller.  Pathname copies are not atomic.  Thus, unless there
> is a bug in RtlGetCurrentDirectory_U(), it has some guarantee that
> other threads will not modify the pathname in its VistaCwd instance,
> though they are free to allocate a new VistaCwd instance and assign
> its address to the global Cwd pointer (so long as they lock CwdCS).
> 
> Presumably the point of the Vista++ CWD mechanism is to reduce
> contention on the CWD lock.  Threads acquire that lock for very
> short periods of time.  They appear to avoid making system calls
> or even allocating memory while holding it.  Also, the CWD lock
> is no longer the PEB lock, so CWD usage does not serialize with
> other PEB-related activities.
> 
> Anyway, it looks to me as if overwriting the path in an existing
> VistaCwd instance would confuse RtlGetCurrentDirectory_U() and
> probably other Win32 API functions.  (It may be that the same is
> true for the handle member; I have not tried to find that out.)
> Cygwin should probably allocate a new VistaCwd instance,
> as does the Win32 SetCurrentDirectory() implementation.

Hmm, I'm still wondering.  In theory we don't have to replace the
directory name.  We just have to InterlockedExchange the handle, since
we only need to replace the original handle which does not allow
sharing-for-delete with our own handle which does.

All other border cases (virtual dir, directory with restricted rights)
can be rightfully handled by setting the Win32 CWD to \\?\PIPE\ again.

That's at least worth a try, isn't it?

> > >        << "  <== critical section" << endl;
> > >   cout << showbase << hex << (size_t)Cwd
> > >        << "  <== Vista++ CWD struct pointer" << endl;
> > 
> > Is there any connection between these two addresses, like, say, they
> > are side by side?
> 
> Not that I have been able to find.  The gap between Cwd and CwdCS
> is always positive, but is large and varies among OS versions:

Bummer.  I was hoping that these global variables could be identified
as part of some other global structure for which there is an easier way
to find it's address.

> > Can't we find Cwd by scanning the .data segment of ntdll.h for the
> > address we inferred from the Params.CurrentDirectoryName.Buffer value?
> 
> Nice; that might work.  But there would need to be some kind of rule
> to pick a winner among multiple matching words, in case by coincidence
> some other non-pointer word just happens to have the same bit pattern.
> I can see two alternatives for the multiple-match case:
> 
>   1. Code scanning, as suggested earlier.  There would need to be a
>   unit test of the code scanner itself so that incompatible changes to
>   ntdll.dll could be detected deterministically, instead of only after
>   a multiple-match coincidence.
> 
>   2. Call SetCurrentDirectory() and recheck the previously-matching
>   addresses to see which one matches the new value.  Place some limit
>   (like 4) on the number of such retries, in case some new version of
>   ntdll.dll intentionally duplicates the pointer every time.
>   (Not sure what to do in that case; fall back to code scanning?)

Sounds like code scanning is the better choice then.  Your code scanner
isn't overly complicated anyway, and it has the advantage of being
rather fast.


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