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

[Corinna Vinschen]

On Sep 14 17:39, John Carey wrote:
> On Sep 14 01:12, Corinna Vinschen wrote:
> > On Sep 13 19:47, John Carey wrote:
> > > On Sep 11 03:30, Corinna Vinschen:
> > > 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?
> 
> Oh, I had thought that you wanted it for long paths
> and directories with restricted rights, too.

That would be nice, but to me it looks like the Win32 API is just too
restricted for that, so that it won't make much sense.  The native API
can't make any assumption that it would work in such a scenario.

Most Win32 calls will still not work in a restricted rights CWD, because
they neglect to set the FILE_OPEN_FOR_BACKUP_INTENT flag.  And most Apps
don't set the FILE_FLAG_BACKUP_SEMANTICS flag incalls to CreateFile.

As for long paths, there's some chance that the internal calls creating
an absolute path from a relative path fail or crash if the CWD is longer
than MAX_PATH.  What MSDN has to say about the maximum path length of
relative paths(*) is not very promising, at least.  Also, even if
Vista++ allows to create VistaCwd entries with a longer path, this would
probably break starting native child processes due to the MAX_PATH
restriction for the CWD in calls to CreateProcess.

> There's one other issue: the new directory will limit deletion
> very briefly, until replaced.  I don't know if that will bother
> people as much as the current situation.

True.  Implementing a full replacement for SetCurrentDirectory as in
your PoC is still an option.  However, I can't do that anymore since
I'm tainted by reading your code.  If you would contemplate to sign
a copyright assignment(**), we could create a more thorough solution
with code scanning and all that in the long run.

In the meantime, I could apply my patch and we can try how well it
works, hacked as it is.


Corinna


(*) http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx

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