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

[Michael Haubenwallner <michael dot haubenwallner at ssi-schaefer dot com>]

Hi Corinna,

On 12/08/2015 10:48 PM, Corinna Vinschen wrote:
> Hi Michael,
> 
> sorry for the delay, but I have some other duties besides Cygwin and the
> W10 1511 problem has thrown me back even more, so I only now find the
> time to review your patch.

Never mind, same here ;)

> On Nov 16 19:07, Michael Haubenwallner wrote:

>> BTW: MSDN document for FILE_INTERNAL_INFORMATION structure tells about
>> IndexNumber as the member - and so does /usr/include/w32api/, but
>> winsup/cygwin/ntdll.h does name it FileId - caused me some confusion...
>> Is that intentionally?
> 
> Fixed now.

Thanks!

> First off, before I dive into the patch:
> 
> 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.

Accepted, fine with me!
've thought of splitting the patch along something like:
* declare additional struct members
* define additional methods as noop
* call new noop methods in existing code
* allocate additional memory
* open additional handles
* implement preparation methods
* implement nomination methods
* synchronize (use semaphore)
* implement cleanup
* implement hardlink-creation
* activate hardlink-creation
* probably more...

Will post them to -patches then.

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

>> +/* Use this buffer under loader lock conditions only. */
>> +PWCHAR
>> +dll::nt_max_path_buf ()
>> +{
>> +  static WCHAR NO_COPY buf[NT_MAX_PATH];
>> +  return buf;
>> +}
> 
> Shouldn't buf (with different name then perhaps) be a private static
> member of dll_list instead?  dll::nt_max_path_buf() could become an
> inline function then.

Fine with me.

>> +/* 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.

>> +{
>> +  /* 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?

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?

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.

>> +  else
>> +  if (!wcsncmp (*nameptr, L"\\??\\", 4))
> 
> Small style nit:  if else in a single line, please.

ok.

>> +  HANDLE fh = NULL;
>> +  ULONG share = FILE_SHARE_VALID_FLAGS;
>> +  IO_STATUS_BLOCK iosb;
>> +  *pstatus = NtOpenFile (&fh, access, &oa, &iosb, share, openopts);
>> +  debug_printf ("%y = NtOpenFile (%p, a %xh, sh %xh, o %xh, io %y, '%W')",
>> +      *pstatus, fh, access, share, openopts, iosb.Status, fn.Buffer);
>> +
>> +  return fh;
> 
> Am I too paranoid?  AFAICS, NtOpenFile doesn't give a guarantee that
> the handle stays unchanged if the function fails.  Wouldn't be something
> like 
> 
>      return NT_SUCCESS (*pstatus) ? fh : NULL;
> 
> be safer here?

Fine with me - you are the experienced one!

>> -      d = (dll *) cmalloc (HEAP_2_DLL, sizeof (*d) + (namelen * sizeof (*name)));
>> +      d = (dll *) cmalloc (HEAP_2_DLL, sizeof (*d) +
>> +	    ((namelen + 1 + forknamesize) * sizeof (*name)));
> 
> Minor style:
> 
>       d = (dll *) cmalloc (HEAP_2_DLL, sizeof (*d)
> 			   + ((namelen + 1 + forknamesize) * sizeof (*name)));

ok.

>> @@ -236,13 +326,24 @@ dll_list::alloc (HINSTANCE h, per_process *p, dll_type type)
>>        wcscpy (d->name, name);
>>        d->modname = d->name + (modname - name);
>>        d->handle = h;
>> -      d->has_dtors = true;
>> +      d->has_dtors = type > DLL_SELF;
> 
> A small comment at this point could be helpful in the long run.

Indeed.

>> +      LARGE_INTEGER zero = { 0 };
>> +      d->fii.FileId = zero;
> 
> Oops, sorry, FileId -> NameIndex :}

IndexNumber, actually.

>> @@ -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?
Fine with me.

>> @@ -61,7 +62,13 @@ struct dll
>>    DWORD image_size;
>>    void* preferred_base;
>>    PWCHAR modname;
>> -  WCHAR name[1];
>> +  /* forkable { */
> 
> Please drop these bracketing comments, especially the braces in them.
> If you want to emphasize them, just use an introductory comment and
> an empty line at the end of the block.

ok.

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

>> +	case L'?':
>> +	case L':':
>> +	  *(target++) = L'_';
> 
> This parens are not required.  Please remove them.

ok.

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

Feels hard - but possible - to utilize the static buffer for recursive
NtQueryDirectoryFile instead. Or add a second static buffer?

>> +  for (++i; i <= lastsepcount; ++i)
>> +    {
>> +      if (success && (i == 0 || wcslen (wcsrchr (dirname, L'\\')) > 1))
>> +	{
>> +	  BOOL ret = CreateDirectoryW (dirname, psec);
> 
> NtCreateFile?

ok.

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

>> +/* Create the nominated hardlink for one indivitual dll,
>> +   inside another subdirectory when dynamically loaded. */
>> +bool
>> +dll::create_forkable ()
>> +{
>> 
>> +  if (type >= DLL_LOAD)
>> +    {
>> +      last = wcsrchr (forkable_name, L'\\');
>> +      if (!last)
>> +        return false;
>> +      *last = L'\0';
>> +      success = mkdirs (forkable_name, &sec_none_nih, 1);
> 
> What's the uppermost directory which will be created here?

The idea is to control this via the 'create_dir' member of forkable_nameparts[].
Currently /var/run/cygfork must exist, so /var/run/cygfork/<sid> will be created first.

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

> 
>> +/* Into buf if not NULL, write the ntname of cygwin installation_root.
>> +   Return the number of characters (that would be) written. */
>> +int
>> +dll_list::rootname (PWCHAR buf)
>> +{
>> +  PWCHAR cygroot = cygheap->installation_root;
>> +  if (!buf)
>> +    return wcslen (cygroot);
>> +
>> +  dll::to_ntname (&cygroot, dll::nt_max_path_buf ());
>> +  wcscpy (buf, cygroot);
>> +  return wcslen (buf);
> 
>      return wcpcpy (buf, cygroot) - buf;

ok.

> Alternatively we could add a cygheap->installation_root_len member.

Feels like subsequent optimization - beyond this feature patch for now.

>> +int
>> +dll_list::sidname (PWCHAR buf)
>> +{
>> 
>> +  wcscpy (buf, sid.Buffer);
>> +  return wcslen (buf);
> 
> See above.

ok.

>> +int
>> +dll_list::ctimename (PWCHAR buf)
>> +{
>> 
>> +	  /* Relevant FILE_BASIC_INFORMATION members are
>> +	     valid in child when cygheap-copied from parent,
>> +	     so caching them is fine.
>> +	     The FILE_BASIC_INFORMATION queried here also is
>> +	     used in dll_list::update_forkables_needs below. */
>> +	  NTSTATUS status;
>> +	  IO_STATUS_BLOCK iosb;
>> +	  status = NtQueryInformationFile (d->fhandle, &iosb,
>> +					   &d->fbi, sizeof (d->fbi),
>> +					   FileBasicInformation);
>> +	  if (!NT_SUCCESS (status))
>> +	    {
>> +	      system_printf ("WARNING: %y = NtQueryInformationFile (%p,"
>> +			     " BasicInfo, io.Status %y, %W)",
>> +			     status, d->fhandle, iosb.Status, d->name);
>> +	      d->fbi.FileAttributes = 0;
>> +	      d->fbi.LastWriteTime = zero;
>> +	    }
>> +	}
>> +
>> +      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, while LastWriteTime
seems to more properly tell about the last file-content modification.
Added a comment.

>> +/* 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?

>> +    success = false;
>> +
>> +  if (success)
>> +    {
>> +      /* create the DotLocal file as empty file */
>> +      wcsncat (buf, main_executable->modname, NT_MAX_PATH);
>> +      wcsncat (buf, L".local", NT_MAX_PATH);

wcpncat missing?

>> +      HANDLE hlocal = CreateFileW (buf, GENERIC_WRITE,
>> +				   FILE_SHARE_READ, &sec_none_nih,
>> +				   OPEN_ALWAYS, 0, NULL);
> 
> Please use NtCreateFile.  We're using CreateFile only for the console
> and in one instance for pipes.

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?

Thanks a lot!
/haubi/