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.
Subterfuge aside, I have a few questions and comments, see below.
Plesae note that I didn't apply the patch so far but just inspected it.
Post by Michael HaubenwallnerPost by Corinna VinschenI'm not yet clear on how this is going to handle binaries
installed to another drive...
For now, when NtSetInformationFile (FileLinkInformation) does fail
for whatever reason (I cannot think of anything else than another
drive), this file is not hardlinked and will be used from its original
location. Must admit I haven't tested this scenario.
* Either create some cygfork-directory on each drive to hold the
hardlinks (still requires NTFS), and put symlinks into /var/run/
* or do a file-copy using the pre-opened handle into /var/run/
(should work even with FAT, but would be slow).
Indeed. Let's stick to the DLLs from the distro which are not supposed
to be on another drive. /var/run and hardlinks should do. This
functionality won't be supported on FAT and I don't see a reason to.
Post by Michael HaubenwallnerBTW: 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.
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.
Another thing occured to me: Doesn't using this stuff break the output
of /proc/<PID>/maps?
Post by Michael Haubenwallner+/* 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.
Post by Michael Haubenwallner+/* 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?
Post by Michael Haubenwallner+{
+ /* 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.
Post by Michael Haubenwallner+ {
+ ntname = ntnamebuf;
+ memmove (ntname + 4, *nameptr,
+ sizeof (*nameptr) * min (NT_MAX_PATH - 4, wcslen (*nameptr) + 1));
+ wcsncpy (ntname, L"\\??\\", 4);
+ ntname[NT_MAX_PATH - 1] = L'\0';
+ *nameptr = ntname + 4;
+ }
+ else
+ if (!wcsncmp (*nameptr, L"\\??\\", 4))
Small style nit: if else in a single line, please.
Post by Michael Haubenwallner+ *nameptr += 4;
+ return ntname;
+}
+ [...]
+HANDLE
+dll::ntopenfile (PWCHAR ntname, NTSTATUS *pstatus, ULONG openopts)
+{
+ NTSTATUS status;
+ if (!pstatus)
+ pstatus = &status;
+
+ UNICODE_STRING fn;
+ RtlInitUnicodeString (&fn, ntname);
+
+ OBJECT_ATTRIBUTES oa;
+ InitializeObjectAttributes (&oa, &fn, 0, NULL, NULL);
+
+ ACCESS_MASK access = FILE_READ_ATTRIBUTES;
+ if (openopts & FILE_DELETE_ON_CLOSE)
+ access |= DELETE;
+ if (openopts & FILE_DIRECTORY_FILE)
+ access |= FILE_LIST_DIRECTORY;
+
+ access |= SYNCHRONIZE;
+ openopts |= FILE_SYNCHRONOUS_IO_NONALERT;
+
+ 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?
Post by Michael Haubenwallnerelse
{
+ size_t forknamesize = forkable_namesize (type, name, modname);
+
/* FIXME: Change this to new at some point. */
- 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)));
Post by Michael Haubenwallner@@ -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.
Post by Michael Haubenwallnerd->p = p;
d->ndeps = 0;
d->deps = NULL;
d->image_size = ((pefile*)h)->optional_hdr ()->SizeOfImage;
d->preferred_base = (void*) ((pefile*)h)->optional_hdr()->ImageBase;
d->type = type;
+ d->forkable_name = d->name + namelen + 1;
+ *d->forkable_name = L'\0';
+ d->fbi.FileAttributes = INVALID_FILE_ATTRIBUTES;
+ LARGE_INTEGER zero = { 0 };
+ d->fii.FileId = zero;
Oops, sorry, FileId -> NameIndex :}
Post by Michael Haubenwallner+ PWCHAR ntname = dll::to_ntname (&name, dll::nt_max_path_buf ());
+ NTSTATUS status;
+ d->fhandle = dll::ntopenfile (ntname, &status);
+ if (!d->fhandle)
+ system_printf ("Unable (ntstatus %y) to open file %W",
+ status, ntname);
append (d);
if (type == DLL_LOAD)
loaded_dlls++;
@@ -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?
Post by Michael Haubenwallnerdiff --git a/winsup/cygwin/dll_init.h b/winsup/cygwin/dll_init.h
index 8127d0b..12344de 100644
--- a/winsup/cygwin/dll_init.h
+++ b/winsup/cygwin/dll_init.h
@@ -43,6 +43,7 @@ struct per_module
typedef enum
{
DLL_NONE,
+ DLL_SELF, /* main-program.exe, cygwin1.dll */
DLL_LINK,
DLL_LOAD,
DLL_ANY
@@ -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.
Post by Michael Haubenwallnerdiff --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...
Post by Michael Haubenwallner+/* Mangle full srcpath as one single filename into targetbuf,
+ optionally telling the last path separator to allow for
+ restoring the file's basename.
+ Return value is the number of characters mangled. */
+static int
+mangle_as_filename (PWCHAR targetbuf, PCWCHAR srcpath, PWCHAR *lastpathsep)
+{
+ PWCHAR target = targetbuf;
+ if (lastpathsep)
+ *lastpathsep = NULL;
+
+ for (; *srcpath; ++srcpath)
+ switch (*srcpath)
+ {
+ if (lastpathsep)
+ *lastpathsep = target;
+ *(target++) = L',';
+ break;
+ *(target++) = L'_';
This parens are not required. Please remove them.
Post by Michael Haubenwallner+ break;
+ *(target++) = *srcpath;
+ break;
+ }
+ return target - targetbuf;
+}
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.
Post by Michael Haubenwallner+/* Create the lastsepcount directories found in dirname, where
+ counting is done along path separators (including trailing ones).
+ Returns true when these directories exist afterwards, false otherways.
+ The dirname is used for the path-splitting. */
+static
+bool mkdirs (PWCHAR dirname, SECURITY_ATTRIBUTES *psec, int lastsepcount)
+{
+ bool success = true;
+ int i = lastsepcount;
+ for (--i; i > 0; --i)
+ {
+ PWCHAR lastsep = wcsrchr (dirname, L'\\');
+ if (!lastsep)
+ break;
+ *lastsep = L'\0';
+ }
+
+ for (++i; i <= lastsepcount; ++i)
+ {
+ if (success && (i == 0 || wcslen (wcsrchr (dirname, L'\\')) > 1))
+ {
+ BOOL ret = CreateDirectoryW (dirname, psec);
NtCreateFile?
Post by Michael Haubenwallner+ if (!ret && GetLastError () != ERROR_ALREADY_EXISTS)
+ success = false;
+ debug_printf ("%d = CreateDirectoryW (%W) %E", ret, dirname);
+ }
+ if (i < lastsepcount)
+ dirname[wcslen (dirname)] = L'\\'; /* restore original value */
+ }
+ return success;
+}
[...]
+/* Nominate the hardlink to an individual DLL inside dirx_name,
+ that ends with the path separator (hence the "x" varname).
+ With NULL as dirx_name, never nominate the hardlink any more.
+ With "" as dirx_name, denominate the hardlink. */
+void
+dll::nominate_forkable (PCWCHAR dirx_name)
+{
+ if (!dirx_name)
+ {
+ debug_printf ("type %d disable %W", type, name);
+ forkable_name = NULL; /* never create a hardlink for this dll */
+ }
+
+ if (!forkable_name)
+ return;
+
+ wcscpy (forkable_name, dirx_name);
Suggestion: Use
PWCHAR p = wcpcpy (forkable_name, dirx_name);
You can use p later on...
Post by Michael Haubenwallner+
+ if (!*forkable_name)
+ return; /* denominate */
+
+ if (type < DLL_LOAD)
+ wcscat (forkable_name, modname);
...here as in
wcpcpy (p, modname);
Post by Michael Haubenwallner+ else
+ {
+ * mangle full path into one single directory name,
+ * just keep original filename intact. The original
+ * filename is necessary to serve as linked
+ * dependencies of dynamically loaded dlls. */
+ PWCHAR lastpathsep;
+ mangle_as_filename (forkable_name + wcslen (forkable_name), name,
+ &lastpathsep);
...and here as in
mangle_as_filename (p, name, &lastpathsep);
This avoids calling wcslen twice.
Post by Michael Haubenwallner+ if (lastpathsep)
+ *lastpathsep = L'\\';
+ }
+}
+
+/* Create the nominated hardlink for one indivitual dll,
+ inside another subdirectory when dynamically loaded. */
+bool
+dll::create_forkable ()
+{
+ if (!forkable_name || !*forkable_name)
+ return true; /* disabled */
+
+ if (!fhandle)
+ return false;
+
+ PWCHAR last = NULL;
+ bool success = true;
+ 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? Is it safe to
use sec_none_nih for all of them?
Post by Michael Haubenwallner+/* 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;
Alternatively we could add a cygheap->installation_root_len member.
Post by Michael Haubenwallner+}
+
+/* Into buf if not NULL, write the string representation of current user sid.
+ Return the number of characters (that would be) written. */
+int
+dll_list::sidname (PWCHAR buf)
+{
+ if (!buf)
+ return 128;
+
+ UNICODE_STRING sid;
+ WCHAR sidbuf[128+1];
+ RtlInitEmptyUnicodeString (&sid, sidbuf, sizeof (sidbuf));
+ RtlConvertSidToUnicodeString (&sid, cygheap->user.sid (), FALSE);
+ wcscpy (buf, sid.Buffer);
+ return wcslen (buf);
See above.
Post by Michael Haubenwallner+int
+dll_list::ctimename (PWCHAR buf)
+{
+ if (!buf)
+ return 16;
+
+ LARGE_INTEGER zero = { 0 };
+ LARGE_INTEGER newest = { 0 };
+ /* Need by-handle-file-information for _all_ loaded dlls,
+ as most recent ctime forms the hardlinks directory. */
+ dll *d = &dlls.start;
+ while ((d = d->next))
+ {
+ if (!d->fhandle)
+ continue;
+ if (d->fbi.FileAttributes == INVALID_FILE_ATTRIBUTES)
+ {
+ /* 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?
Post by Michael Haubenwallner+/* Create the nominated forkable hardlinks and directories as necessary,
+ as well as the .local file for dll-redirection. */
+bool
+dll_list::create_forkables ()
+{
+ bool success = true;
+
+ int lastsepcount = 1; /* we have trailing pathsep */
+ for (namepart const *part = forkable_nameparts; part->text; ++part)
+ if (part->create_dir)
+ ++lastsepcount;
+
+ PWCHAR buf = dll::nt_max_path_buf ();
+ wcsncpy (buf, forkables_dirx_name, NT_MAX_PATH);
+
+ if (!mkdirs (buf, &sec_none_nih, lastsepcount))
Again, sec_none_nih for the entire directory tree?
Post by Michael Haubenwallner+ 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);
+ 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.
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.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat