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] |
Hi Michael, 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. > > 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. Thanks, but splitting into a handful separately functional patches should be sufficient. > > 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. 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... > >> +/* 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... > 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? > >> + 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. > >> +/* 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. > > 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. 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. 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. 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... Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat
Attachment:
signature.asc
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |