This is the mail archive of the cygwin-developers mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Protect fork() against dll- and exe-updates.


(back to this topic)

On 01/28/2016 07:43 PM, Corinna Vinschen wrote:
> On Jan 26 17:23, Michael Haubenwallner wrote:
>> On 12/08/2015 10:48 PM, Corinna Vinschen wrote:
>>> On Nov 16 19:07, Michael Haubenwallner wrote:
>> [...]
>>> Would you mind terribly to split this patch into a patchset so we
>>> get a set of smaller patches as far as it makes sense?  I'm a bit
>>> reluctant to apply such a big patch in one go.  I did this myself
>>> a lot back in the pre-git CVS times, but the longer I work with git
>>> the more I appreciate patches split into sets of smaller patches.
>>> They are easier to review and *much* easier to handle when bisecting
>>> the code in case of searching for a bug.

>> Will post them to -patches then.

split and updated patches on the way now.

>>> Another thing occured to me:  Doesn't using this stuff break the output
>>> of /proc/<PID>/maps?
>>
>> Whenever the original binary has been removed - it is moved to trashbin
>> actually, it turns out that /proc/<PID>/maps shows the trashbin-filename.
>> Not sure if you call this "broken" - or what would be "unbroken" here.
> 
> No, what I mean is, what does it show if the file has *not* been deleted
> yet?  The path to /usr/bin/foo or the path to /var/run/cygfork/<SID>/foo?
> The latter case would be rather irritating.

Haven't managed to see the /var/run/cygfork/ path in /proc/*/maps.

> This behaviour may not be that bad in case of running with a deleted
> object, though.  On Linux /proc/$PID/maps prints the name of a deleted
> object with a "(deleted)" tag.  Maybe we can get there, too.  Do we have
> the information from where the file has been originally loaded still
> available at that point?  I guess the answer is no...

When Windows moves some file into trash, IMHO there is some extra info
file containing the original path. But with Cygwin unlink() ?

>>>> +/* Return *nameptr probably prefixed with "\\??\\", but
>>>> +   update *nameptr to stay without the "\\??\\" prefix. */
>>>> +PWCHAR
>>>> +dll::to_ntname (PWCHAR *nameptr, WCHAR ntnamebuf[NT_MAX_PATH])
>>>
>>> Why does dll::to_ntname need a second parameter if it's always called
>>> on the buffer returned by dll::nt_max_path_buf?
>>
>> This merely is to indicate that it does need some buffer. When removing
>> the second parameter, I'd rename to something like dll::buffered_ntname.
> 
> ack
> 
>>>> +{
>>>> +  /* avoid using path_conv here: cygheap might not be
>>>> +     initialized when started from non-cygwin process,
>>>> +     or still might be frozen in_forkee */
>>>> +  PWCHAR ntname = *nameptr;
>>>> +  if ((*nameptr)[1] == L':')
>>>
>>> What if Cygwin has been installed on a network drive with no drive
>>> letter attached?  In that case you'd have to deal with UNC paths,
>>> but they are ignored here.
>>
>> Isn't the UNC prefix cut off right after GetModuleFileNameW in
>> dll_list::alloc?
> 
> Not necessarily.  GetModuleFileNameW returns \\?\ paths as well.  It
> depends on how the executable has been loaded.
> 
>> Actually, I didn't fully grok the subtleties along "\\?\", "\??\",
>> "UNC\", "\\" and their usage differences with functions from
>> kernel32.dll and ntdll.dll yet.  Is it possible in every case that
>> ntdll just may need the additional prefix "\??\", while kernel32 takes
>> the same path without that prefix?
> 
> It's not very complicated, just puzzeling at first:
> 
> Short Win32 paths:
> 
> Drive letter path:   C:\foo\bar
> UNC path:            \\server\share\baz
> 
> Equivalent long Win32 paths:
> 
> Drive letter path:   \\?\C:\foo\bar
> UNC path:            \\?\UNC\server\share\baz
> 
> Equivalent native NT paths:
> 
> Drive letter path:   \??\C:\foo\bar
> UNC path:            \??\UNC\server\share\baz
> 
> Note 1: Long Win32 paths are identical to internal NT paths, except
> 	for the first question mark replaced by a backslash.  Why?
> 	I have no idea.  History, probably.
> 
> Note 2: Short Drive letter paths simply get a "\??\" prepended to be
> 	converted to NT paths.
> 
> Note 3: UNC paths get their first backslash replace with "\??\UNC",
>         so one backslash is lost on the way to the native NT form.
> 
> Note 4: "\??\" is just the name of a(*) virtual directory in the native
> 	NT namespace which contains symlinks to the actual devices.  The
> 	"winobj" tool from sysinternals is quite instructive.  Exposure
> 	to the Win32 world is performed by the QueryDosDevices and
> 	DefineDosDevice calls.
> 
> 	(*) Actually more than one, what with system and user symlinks
> 	    merged into a single view...

Thanks, this is very helpful!

>> I do think of storing only the ntdll-name in the structures, and have a non-
>> prefix name pointer into that ntdll-name buffer for use by CreateProcess&co then.
> 
> Good idea in general.  Just the UNC path problem above is slightly in
> the way.
> 
>>>> +      LARGE_INTEGER zero = { 0 };
>>>> +      d->fii.FileId = zero;
>>>
>>> Oops, sorry, FileId -> NameIndex :}
>>
>> IndexNumber, actually.
> 
> Duh, right.
> 
>>>> @@ -396,7 +497,7 @@ dll_list::detach (void *retaddr)
>>>>    if (!myself || in_forkee)
>>>>      return;
>>>>    guard (true);
>>>> -  if ((d = find (retaddr)))
>>>> +  if ((d = find (retaddr)) && d->type != DLL_SELF)
>>>
>>> dll_list::find is only ever used to find dlls with d->type != DLL_SELF.
>>> Wouldn't it make sense to move this check into the method?
>>
>> You mean into the dll_list::find method?
> 
> Yep.
> 
>>>> diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
>>>> index 084f8f0..dd0f9a6 100644
>>>> --- a/winsup/cygwin/fork.cc
>>>> +++ b/winsup/cygwin/fork.cc
>>>> @@ -356,8 +351,19 @@ frok::parent (volatile char * volatile stack_here)
>>>>  
>>>>    while (1)
>>>>      {
>>>> +      dlls.request_forkables ();
>>>
>>> I'm aware that hold_everything has been called but, is that safe?
>>> request_forkables calls update_forkables_needs which in turn uses the
>>> static dll::nt_max_path_buf buffer...
>>
>> While I have a vague idea only on what hold_everything actually does,
>> isn't one of the ideas for the static buffer to be useable even while
>> everything else (heap in particular) is on hold?
>> Especially as that static buffer does have the NO_COPY attribute...
> 
> Yeah, right.
> 
>>> So what happens with a path where the parent dir path is > NAME_MAX?
>>> If I didn't miss something, this won't work for very long paths.
>>
>> Reading through the docs I've got the impression that while NT_MAX_PATH is
>> to hold a very long path, a single path part is still limited to NAME_MAX,
>> but I may easily be wrong here.
> 
> Well, yes, NAME_MAX is the maximum length of a single path component.
> But the comment preceeding mangle_as_filename says:
> 
>   "Mangle full srcpath as one single filename ..."
> 
> The function replaces backslashes with commas.  So, IIUC, an incoming
> path consisting of, e.g., a dir with NAME_MAX length, a backslash, and
> a filename with NAME_MAX length will be converted to an invalid path
> with a single path component of 2 * NAME_MAX + 1 (comma) length.  What
> am I missing?

Nice catch (out-of-coffee exception)!

I'm using the main executable's IndexNumber now, and the original
directory's IndexNumber for the directory holding the hardlink for
a dynamically loaded dll.

>>>> +  wcscpy (forkable_name, dirx_name);
>>>
>>> Suggestion: Use
>>>
>>>      PWCHAR p = wcpcpy (forkable_name, dirx_name);
>>>      
>>> You can use p later on...
>>
>>> ...and here as in
>>>
>>>          mangle_as_filename (p, name, &lastpathsep);
>>>
>>> This avoids calling wcslen twice.
>>
>> ok, 've not seen wcpcpy before.
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/wcpcpy.html
> 
>>> Is it safe to use sec_none_nih for all of them?
>>
>> As long as /var/run/cygfork exists with tmpdir-like permission,
>> sec_none_nih seems fine: For the directories created, ls -l shows
>> 'drwxr-xr-x+' permissions.
> 
> That's inherited from the parent.  Hmm, ok, yeah, that sounds like
> the right thing.  We can revisit it later if a need arises for some
> reason.
> 
>>> Alternatively we could add a cygheap->installation_root_len member.
>>
>> Feels like subsequent optimization - beyond this feature patch for now.
> 
> Ok.
> 
>>>> +      if (d->fbi.LastWriteTime.QuadPart > newest.QuadPart)
>>>> +	newest = d->fbi.LastWriteTime;
>>>
>>> LastWriteTime?  This is supposed to check if a newer DLL replaced
>>> an older in-use one, so shouldn't that be CreationTime?
>>
>> CreationTime bumps even when creating a hardlink,
> 
> Not on my machine.  Observe the "Birth" date in stat after creating a
> hardlink to a file:
> 
>   $ stat tsock.c | grep Birth
>    Birth: 2016-01-11 18:57:13.584871900 +0100
>   $ ln tsock.c tsock.link
>   $ stat tsock.c | grep Birth
>   $ stat tsock.c tsock.link | grep Birth
>    Birth: 2016-01-11 18:57:13.584871900 +0100
>    Birth: 2016-01-11 18:57:13.584871900 +0100
> 
> Are you confusing CreationTime with ChangeTime by any chance?
> 
>> while LastWriteTime
>> seems to more properly tell about the last file-content modification.
> 
> LastWriteTime might be sufficient, but the file is actually recreated
> when replacing it, it's not overwritten, that's why CreationTime sounds
> more correct to me.  Unfortunately CreationTime is as much unsafe
> against tampering as is LastWriteTime, so never mind.
> 
> A bit awkward is the name of the method, though.  Ctime is the POSIX
> equivalent for ChangeTime, not for LastWriteTime.  Either mtime when
> using LastWriteTime or birthtime when using CreationTime would be
> better, methinks.

I've renamed to lwtime (LastWriteTime) now. ctime is the initial name
when I started with ChangeTime, which bumps on hardlink creation - not
sure if I really tried CreationTime.

>>>> +/* Create the nominated forkable hardlinks and directories as necessary,
>>>> +   as well as the .local file for dll-redirection. */
>>>> +bool
>>>> +dll_list::create_forkables ()
>>>> +{
>>>>
>>>> +  if (!mkdirs (buf, &sec_none_nih, lastsepcount))
>>>
>>> Again, sec_none_nih for the entire directory tree?
>>
>> As I didn't fully grok the windows security too: What is your concern here?
> 
> Not sure.  I was just mulling over the default perms resulting from
> it being inadaquate.  Let's just keep it in mind.

Ok.

>>> What's missing is user documentation for this feature.  A bit of
>>> description what happens, what has to be done by the user, and what
>>> limitations it has would be helpful.
>>
>> Agreed: Where to add such docs?
> 
> Good question.  The docs have grown a bit wild...  maybe create a chapter
> in pathnames.xml for now.  We can move it around later.

I've identified faq-api.xml and hightlights.xml, near the existing
fork docs...

> There's one point I forgot when I reviewed the patch last year.  You're
> doing the check if /var/run/cygfork exists in dll_list::create_forkables
> once per process, per loaded DLL.  Why?  In theory, if you found out
> that you can't create a hardlink once, you're done.

The idea is to re-check once per fork() call when it was available before,
to not necessarily break when /var/run/cygfork/ is removed...

> What I'd like to see is that failing to hardlink a file the first time
> *because /var/run/cygfork does not exist* results in setting a flag in
> cygheap so that no other process in the process tree ever tries to use
> this feature again.  An explicit check for existence of the dir seems
> necessary at some early stage in the code.  Creating the directory and
> exiting Cygwin processes is required to activate the feature then, but
> that's ok.  The impact on users not using this feature should be as low
> as possible.

Ok.

> I'm still not overly excited about using the existence of the dir alone
> as marker to activate this feature, but that can be added later...

I could think of some flag to set in setup.exe, but still where to store
the flag except for the existence of the directory. I'm not sure if the
registry should be used for everything...
 
Thanks!
/haubi/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]