Re: Extending /proc/*/maps
Hi Ryan, On May 11 01:27, Ryan Johnson wrote: > Hi all, > > Please find attached three patches which extend the functionality of > /proc/*/maps. Thanks! I applied youyr two first patches with a couple of changes: - Formatting: Setting of curly braces in class and method defintions, a lot of missing spaces, "int *foo" instead of "int* foo", stuff like that. Please compare what I checked in against your patch. That doesn't mean I always get it right myself, but basically that's how it should be. - NT_MAX_PATH is the maximum size of a path including the trailing \0, so a buffer size of NT_MAX_PATH + 1 isn't required. - Please always use sys_wcstombs rather than wcstombs for filenames. - I replaced the call to GetMappedFileNameEx with a call to NtQueryVirtualMemory (MemorySectionName). This avoids to add another dependency to psapi. I intend to get rid of them entirely, if possible. > NOTE 1: I do not attempt to identify PEB, TEB, or thread stacks. The > first could be done easily enough, but the second and third require > venturing into undocumented/private Windows APIs. Go ahead! We certainly don't shy away from calls to NtQueryInformationProcess or NtQueryInformationThread. > NOTE 2: If desired, we could easily extend format_process_maps > further to report section names of mapped images (linux does this > for .so files), [...] Sorry if I'm dense, but isn't that exactly what GetMappedFileNameEx or NtQueryVirtualMemory (MemorySectionName) do? I also don't see any extra information for .so files in the Linux maps output. What detail am I missing? Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
Re: Extending /proc/*/maps
On May 11 01:27, Ryan Johnson wrote:
> The second (proc-maps-heaps) adds reporting of Windows heaps (or
> their bases, at least). Unfortunately there doesn't seem to be any
> efficient way to identify all virtual allocations which a heap owns.
There's a call RtlQueryDebugInformation which can fetch detailed heap
information, and which is used by Heap32First/Heap32Last. Using it
directly is much more efficient than using the Heap32 functions. The
DEBUG_HEAP_INFORMATION is already in ntdll.h, what's missing is the
layout of the Blocks info. I found the info by googling:
typedef struct _HEAP_BLOCK
{
PVOID addr;
ULONG size;
ULONG flags;
ULONG unknown;
} HEAP_BLOCK, *PHEAP_BLOCK;
If this information is searched until the address falls into the just
inspected block of virtual memory, then we would have the information,
isn't it?
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Project Co-Leader cygwin AT cygwin DOT com
Red Hat
Re: Extending /proc/*/maps
On 11/05/2011 6:31 AM, Corinna Vinschen wrote: Hi Ryan, On May 11 01:27, Ryan Johnson wrote: Hi all, Please find attached three patches which extend the functionality of /proc/*/maps. Thanks! I applied youyr two first patches with a couple of changes: - Formatting: Setting of curly braces in class and method defintions, a lot of missing spaces, "int *foo" instead of "int* foo", stuff like that. Please compare what I checked in against your patch. That doesn't mean I always get it right myself, but basically that's how it should be. Sorry... I did my best to match the surrounding code but didn't notice the '* ' vs ' *' thing. - NT_MAX_PATH is the maximum size of a path including the trailing \0, so a buffer size of NT_MAX_PATH + 1 isn't required. Good to know. Thanks. - I replaced the call to GetMappedFileNameEx with a call to NtQueryVirtualMemory (MemorySectionName). This avoids to add another dependency to psapi. I intend to get rid of them entirely, if possible. Nice. One issue: I tried backporting your change to my local tree, and the compiler complains that PMEMORY_SECTION_NAME isn't defined; the changelog says you updated ntdll.h to add it, but the online definition in w32api was last updated 9 months ago by 'duda.' Did it perhaps slip past the commit? NOTE 1: I do not attempt to identify PEB, TEB, or thread stacks. The first could be done easily enough, but the second and third require venturing into undocumented/private Windows APIs. Go ahead! We certainly don't shy away from calls to NtQueryInformationProcess or NtQueryInformationThread. Ah. I didn't realize this sort of thing was encouraged. The MSDN docs about them are pretty gloomy. The other things that discouraged me before were: - the only obvious way to enumerate the threads in a process is to create a snapshot using TH32CS_SNAPTHREAD, which enumerates all threads in the system. This sounds expensive. - it's not clear whether GetThreadContext returns a reasonable stack pointer if the target thread is active at the time. However, assuming the above do not bother folks, they should be pretty straightforward to use. I won't have time to code this up in the immediate future, though. My real goal was to make fork bearable on my machine and that ended up sucking away all the time I had and then some... NOTE 2: If desired, we could easily extend format_process_maps further to report section names of mapped images (linux does this for .so files), [...] Sorry if I'm dense, but isn't that exactly what GetMappedFileNameEx or NtQueryVirtualMemory (MemorySectionName) do? I also don't see any extra information for .so files in the Linux maps output. What detail am I missing? Interesting... the machine I used for reference a couple weeks ago was running a really old debian, and for each allocation entry of a mapped image it gave the corresponding section name (.text, .bss, .rdata, etc): 346360-346362c000 r-xp 08:01 2097238 /lib64/libpcre.so.0.0.1 .text 346362c000-346382b000 ---p 0002c000 08:01 2097238 /lib64/libpcre.so.0.0.1 346382b000-346382c000 rw-p 0002b000 08:01 2097238 /lib64/libpcre.so.0.0.1 .bss However, the machine was upgraded to a newer kernel this week and the extra information no longer appears. In any case, should somebody want to report section names within a mapped image, that information can be had easily enough using the pefile struct from my fork patches. Ryan
Re: Extending /proc/*/maps
On May 11 08:53, Ryan Johnson wrote: > On 11/05/2011 6:31 AM, Corinna Vinschen wrote: > >- I replaced the call to GetMappedFileNameEx with a call to > > NtQueryVirtualMemory (MemorySectionName). This avoids to add another > > dependency to psapi. I intend to get rid of them entirely, if > > possible. > Nice. One issue: I tried backporting your change to my local tree, > and the compiler complains that PMEMORY_SECTION_NAME isn't defined; > the changelog says you updated ntdll.h to add it, but the online > definition in w32api was last updated 9 months ago by 'duda.' Did it > perhaps slip past the commit? The compiler shouldn't complain because we only use the definitions from ntdll.h and nothing else from w32api/include/ddk (except in rare cases, see fhandler_tape.cc, fhandler_serial.cc). Sometimes I apply new stuff to w32api as well on an "as it comes along" base, but usually it's not the Cygwin project which maintains these files. > >>NOTE 1: I do not attempt to identify PEB, TEB, or thread stacks. The > >>first could be done easily enough, but the second and third require > >>venturing into undocumented/private Windows APIs. > >Go ahead! We certainly don't shy away from calls to > >NtQueryInformationProcess or NtQueryInformationThread. > Ah. I didn't realize this sort of thing was encouraged. The MSDN > docs about them are pretty gloomy. Gary Nebbett, "Windows NT/2000 Native API Reference". It's a bit rusty as far as new stuff is concerned, but it contains the important stuff we can rely upon. > The other things that discouraged me before were: > - the only obvious way to enumerate the threads in a process is to > create a snapshot using TH32CS_SNAPTHREAD, which enumerates all > threads in the system. This sounds expensive. > - it's not clear whether GetThreadContext returns a reasonable stack > pointer if the target thread is active at the time. Per MSDN the thread context is only valid if the thread is not running at the time. However, we could carefully inspect some of the values and see how stable they are. > However, assuming the above do not bother folks, they should be > pretty straightforward to use. I won't have time to code this up in > the immediate future, though. My real goal was to make fork bearable > on my machine and that ended up sucking away all the time I had and > then some... Well, it's not high enough on my agenda to dive into it. Feel free to do that whenever you find a free time slot. > >Sorry if I'm dense, but isn't that exactly what GetMappedFileNameEx or > >NtQueryVirtualMemory (MemorySectionName) do? I also don't see any extra > >information for .so files in the Linux maps output. What detail am I > >missing? > Interesting... the machine I used for reference a couple weeks ago > was running a really old debian, and for each allocation entry of a > mapped image it gave the corresponding section name (.text, .bss, > .rdata, etc): > 346360-346362c000 r-xp 08:01 2097238 > /lib64/libpcre.so.0.0.1 .text > 346362c000-346382b000 ---p 0002c000 08:01 2097238 > /lib64/libpcre.so.0.0.1 > 346382b000-346382c000 rw-p 0002b000 08:01 2097238 > /lib64/libpcre.so.0.0.1 .bss > > However, the machine was upgraded to a newer kernel this week and > the extra information no longer appears. Indeed, this information is not available on my 2.6.35 kernel. > In any case, should somebody want to report section names within a > mapped image, that information can be had easily enough using the > pefile struct from my fork patches. Right. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
Re: Extending /proc/*/maps
On 11/05/2011 9:20 AM, Corinna Vinschen wrote: On May 11 08:53, Ryan Johnson wrote: On 11/05/2011 6:31 AM, Corinna Vinschen wrote: - I replaced the call to GetMappedFileNameEx with a call to NtQueryVirtualMemory (MemorySectionName). This avoids to add another dependency to psapi. I intend to get rid of them entirely, if possible. Nice. One issue: I tried backporting your change to my local tree, and the compiler complains that PMEMORY_SECTION_NAME isn't defined; the changelog says you updated ntdll.h to add it, but the online definition in w32api was last updated 9 months ago by 'duda.' Did it perhaps slip past the commit? The compiler shouldn't complain because we only use the definitions from ntdll.h and nothing else from w32api/include/ddk (except in rare cases, see fhandler_tape.cc, fhandler_serial.cc). Sometimes I apply new stuff to w32api as well on an "as it comes along" base, but usually it's not the Cygwin project which maintains these files. Ah... there are two ntdll.h, and I looked at the one in w32api/include instead of cygwin. All is well. Ryan
Re: Improvements to fork handling
On 11/05/2011 10:13 AM, Christopher Faylor wrote: On Wed, May 11, 2011 at 09:59:53AM +0200, Corinna Vinschen wrote: On May 11 02:18, Ryan Johnson wrote: Please find attached five patches [...] Oops, wrong mailing list... Btw., it would be nice if you could create patches with the diff -p flag as well. It's not exactly essential, but IMHO it's quite a help when trying to review patches. Another problem is this: While you provide separate patches, you don't provide separate ChangeLogs. That makes it kind of hard to apply them separately. Would you mind to create one ChangeLog per change? Ditto. This really needs to be broken down into easier to review chunks. All right. Let's try this again with the correct mailing list. The patches have been generated with diff -p, and each includes appropriate changelog entries. Hopefully the changes are split up finely enough because I don't know a good way to break them down any further. For posterity's sake I'm including the original message body below. Ryan Please find attached five patches which improve the behavior of forking when Windows isn't cooperating as well we'd like. The results are not as good as I'd originally hoped for, in that it's still entirely possible (even common) for fork attempts to fail, but at least now they are clean failures. Most sources of access violations should be gone, address space clobbers lead to clean child exit, and retries are applied consistently. It will still be important both to rebase and to ASLR-enable dlls, however, because there are too many sources of address space clobbers which we really can't control. One open issue remains: windows dlls, thread stacks, and heaps can and do end up at different locations in the child. This technically breaks fork semantics but I don't know whether we care. Since we currently have no real way to track this or compensate for it in the absence of obvious address space clobbers, the question is probably moot in any case. The first patch (fork-clean-exit) allows a child which failed due to address space clobbers to report cleanly back to the parent. As a result, DLL_LINK which land wrong, DLL_LOAD whose space gets clobbered, and failure to replicate the cygheap, generate retries and dispense with the terminal spam. Handling of unexpected errors should not have changed. Further, the patch fixes several sources of access violations and crashes, including: - accessing invalid state after failing to notice that a statically-linked dll loaded at the wrong location - accessing invalid state while running dtors on a failed forkee. I follow cgf's approach of simply not running any dtors, based on the observation that dlls in the parent (gcc_s!) can store state about other dlls and crash trying to access that state in the child, even if they appeared to map properly in both processes. - attempting to generate a stack trace when somebody in the call chain used alloca(). This one is only sidestepped here, because we eliminate the access violations and api_fatal calls which would have triggered the problematic stack traces. I have a separate patch which allows offending functions to disable stack traces, if folks are interested, but it was kind of noisy so I left it out for now (cygwin uses alloca pretty liberally!). The second (fork-topsort) has the parent sort its dll list topologically by dependencies. Previously, attempts to load a DLL_LOAD dll risked pulling in dependencies automatically, and the latter would then not benefit from the code which "encourages" them to land in the right places. The dependency tracking is achieved using a simple class which allows to introspect a mapped dll image and pull out the dependencies it lists. The code currently rebuilds the dependency list at every fork rather than attempt to update it properly as modules are loaded and unloaded. Note that the topsort optimization affects only cygwin dlls, so any windows dlls which are pulled in dynamically (directly or indirectly) will still impose the usual risk of address space clobbers. The third (fork-reserve-at) fixes a bug in the reserve_at function which caused it to sometimes reserve space needed by the dll it was supposed to help land. This happens when the dll tries to land in a free region which overlaps the desired location. The new code exploits the image introspection to get the dll's image size and avoids the corner cases. The fourth (fork-dll-load) provides a rewrite to dll_list::load_after fork. The new version eliminates reserve_upto() and release_upto(), which were expensive (the process repeats for each dll) and buggy (release_upto could free allocations reserve_upto did not make). Instead, the effect of reserve_upto is achieved by recursively attempting to load each dll in its proper place and calling reserve_at before retrying; each reservation's location is kept on the stack throughout and release_at calls are made only whe
Re: Improvements to fork handling
On Wed, May 11, 2011 at 10:21:14AM -0400, Ryan Johnson wrote: >On 11/05/2011 10:13 AM, Christopher Faylor wrote: >> On Wed, May 11, 2011 at 09:59:53AM +0200, Corinna Vinschen wrote: >>> On May 11 02:18, Ryan Johnson wrote: Please find attached five patches [...] >>> Oops, wrong mailing list... >>> >>> Btw., it would be nice if you could create patches with the diff -p flag >>> as well. It's not exactly essential, but IMHO it's quite a help when >>> trying to review patches. >>> >>> Another problem is this: While you provide separate patches, you don't >>> provide separate ChangeLogs. That makes it kind of hard to apply them >>> separately. Would you mind to create one ChangeLog per change? >> Ditto. This really needs to be broken down into easier to review chunks. >All right. Let's try this again with the correct mailing list. > >The patches have been generated with diff -p, and each includes >appropriate changelog entries. Hopefully the changes are split up finely >enough because I don't know a good way to break them down any further. > >For posterity's sake I'm including the original message body below. Please: One patch per message. One ChangeLog entry per message, not sent as a diff. cgf
Re: Improvements to fork handling
On 11/05/2011 12:16 PM, Christopher Faylor wrote: Please: One patch per message. One ChangeLog entry per message, not sent as a diff. Resend? Or just note for future reference? Ryan
Re: Extending /proc/*/maps
On 11/05/2011 7:14 AM, Corinna Vinschen wrote:
On May 11 01:27, Ryan Johnson wrote:
The second (proc-maps-heaps) adds reporting of Windows heaps (or
their bases, at least). Unfortunately there doesn't seem to be any
efficient way to identify all virtual allocations which a heap owns.
There's a call RtlQueryDebugInformation which can fetch detailed heap
information, and which is used by Heap32First/Heap32Last. Using it
directly is much more efficient than using the Heap32 functions. The
DEBUG_HEAP_INFORMATION is already in ntdll.h, what's missing is the
layout of the Blocks info. I found the info by googling:
typedef struct _HEAP_BLOCK
{
PVOID addr;
ULONG size;
ULONG flags;
ULONG unknown;
} HEAP_BLOCK, *PHEAP_BLOCK;
If this information is searched until the address falls into the just
inspected block of virtual memory, then we would have the information,
isn't it?
The problem is that the reported heap blocks corresponded to individual
malloc() calls when I tested it (unordered list, most sizes < 100B). I
really would have preferred a function that returns the list of memory
regions owned by the heap. They must track that information -- I highly
HeapDestroy uses these heap blocks to decide which memory regions to
VirtualFree, but that info seems to live in the kernel...
Given that Heap32* has already been reverse-engineered by others, the
main challenge would involve sorting the set of heap block addresses and
distilling them down to a set of allocation bases. We don't want to do
repeated linear searches over 50k+ heap blocks.
Also, the cygheap isn't a normal windows heap, is it? I thought it was
essentially a statically-allocated array (.cygheap) that gets managed as
a heap. I guess since it's part of cygwin1.dll we already do sort of
report it...
Ryan
Improvements to fork handling (1/5)
Hi all,
This is the first of a series of patches, sent in separate emails as
requested.
The first patch allows a child which failed due to address space
clobbers to report cleanly back to the parent. As a result, DLL_LINK
which land wrong, DLL_LOAD whose space gets clobbered, and failure to
replicate the cygheap, generate retries and dispense with the terminal
spam. Handling of unexpected errors should not have changed. Further,
the patch fixes several sources of access violations and crashes,
including:
- accessing invalid state after failing to notice that a
statically-linked dll loaded at the wrong location
- accessing invalid state while running dtors on a failed forkee. I
follow cgf's approach of simply not running any dtors, based on the
observation that dlls in the parent (gcc_s!) can store state about other
dlls and crash trying to access that state in the child, even if they
appeared to map properly in both processes.
- attempting to generate a stack trace when somebody in the call chain
used alloca(). This one is only sidestepped here, because we eliminate
the access violations and api_fatal calls which would have triggered the
problematic stack traces. I have a separate patch which allows offending
functions to disable stack traces, if folks are interested, but it was
kind of noisy so I left it out for now (cygwin uses alloca pretty
liberally!).
Ryan
diff --git a/child_info.h b/child_info.h
--- a/child_info.h
+++ b/child_info.h
@@ -92,6 +92,18 @@ public:
void alloc_stack_hard_way (volatile char *);
};
+/* Several well-known problems can prevent us from patching up a
+ forkee; when such errors arise the child should exit cleanly (with
+ a failure code for the parent) rather than dumping stack. */
+#define fork_api_fatal(fmt, args...) \
+ do \
+{ \
+ sigproc_printf (fmt,## args);\
+ fork_info->handle_failure (-1); \
+} \
+ while(0)
+
+
class fhandler_base;
class cygheap_exec_info
diff --git a/dll_init.cc b/dll_init.cc
--- a/dll_init.cc
+++ b/dll_init.cc
@@ -19,6 +19,7 @@ details. */
#include "dtable.h"
#include "cygheap.h"
#include "pinfo.h"
+#include "child_info.h"
#include "cygtls.h"
#include "exception.h"
#include
@@ -131,10 +132,16 @@ dll_list::alloc (HINSTANCE h, per_proces
{
if (!in_forkee)
d->count++; /* Yes. Bump the usage count. */
+ else if (d->handle != h)
+ fork_api_fatal ("Location of %W changed from %p (parent) to %p (child)",
+ d->name, d->handle, h);
d->p = p;
}
else
{
+ if (in_forkee)
+ system_printf ("Unexpected dll loaded during fork: %W", name);
+
/* FIXME: Change this to new at some point. */
d = (dll *) cmalloc (HEAP_2_DLL, sizeof (*d) + (namelen * sizeof
(*name)));
@@ -371,7 +378,6 @@ dll_list::load_after_fork (HANDLE parent
preferred_block = reserve_at (d->name, (DWORD) h);
}
- in_forkee = false;
}
struct dllcrt0_info
diff --git a/dll_init.h b/dll_init.h
--- a/dll_init.h
+++ b/dll_init.h
@@ -57,7 +57,7 @@ struct dll
int init ();
void run_dtors ()
{
-if (has_dtors)
+if (has_dtors && !in_forkee)
{
has_dtors = 0;
p.run_dtors ();
diff --git a/fork.cc b/fork.cc
--- a/fork.cc
+++ b/fork.cc
@@ -233,6 +233,7 @@ frok::child (volatile char * volatile he
sync_with_parent ("loaded dlls", true);
}
+ in_forkee = false;
init_console_handler (myself->ctty >= 0);
ForceCloseHandle1 (fork_info->forker_finished, forker_finished);
@@ -393,10 +394,13 @@ frok::parent (volatile char * volatile s
if (!exit_code)
continue;
this_errno = EAGAIN;
- /* Not thread safe, but do we care? */
- __small_sprintf (errbuf, "died waiting for longjmp before
initialization, "
- "retry %d, exit code %p", ch.retry, exit_code);
- error = errbuf;
+ if (exit_code != EXITCODE_FORK_FAILED)
+ {
+ /* Not thread safe, but do we care? */
+ __small_sprintf (errbuf, "died waiting for longjmp before
initialization, "
+ "retry %d, exit code %p", ch.retry, exit_code);
+ error = errbuf;
+ }
goto cleanup;
}
break;
@@ -515,7 +519,8 @@ frok::parent (volatile char * volatile s
if (!ch.sync (child->pid, pi.hProcess, FORK_WAIT_TIMEOUT))
{
this_errno = EAGAIN;
- error = "died waiting for dll loading";
+ if (ch.exit_code != EXITCODE_FORK_FAILED)
+ error = "died waiting for dll loading";
goto cleanup;
}
diff --git a/heap.cc b/heap.cc
--
Improvements to fork handling (2/5)
Hi all,
This patch has the parent sort its dll list topologically by
dependencies. Previously, attempts to load a DLL_LOAD dll risked pulling
in dependencies automatically, and the latter would then not benefit
from the code which "encourages" them to land in the right places. The
dependency tracking is achieved using a simple class which allows to
introspect a mapped dll image and pull out the dependencies it lists.
The code currently rebuilds the dependency list at every fork rather
than attempt to update it properly as modules are loaded and unloaded.
Note that the topsort optimization affects only cygwin dlls, so any
windows dlls which are pulled in dynamically (directly or indirectly)
will still impose the usual risk of address space clobbers.
Ryan
diff --git a/dll_init.cc b/dll_init.cc
--- a/dll_init.cc
+++ b/dll_init.cc
@@ -116,6 +116,18 @@ dll_list::operator[] (const PWCHAR name)
return NULL;
}
+/* Look for a dll based on is short name only (no path) */
+dll *
+dll_list::find_by_modname (const PWCHAR name)
+{
+ dll *d = &start;
+ while ((d = d->next) != NULL)
+if (!wcscasecmp (name, d->modname))
+ return d;
+
+ return NULL;
+}
+
#define RETRIES 1000
/* Allocate space for a dll struct. */
@@ -152,14 +164,13 @@ dll_list::alloc (HINSTANCE h, per_proces
d->handle = h;
d->has_dtors = true;
d->p = p;
+ d->ndeps = 0;
+ d->deps = NULL;
+ d->modname = wcsrchr (d->name, L'\\');
+ if (d->modname)
+ d->modname++;
d->type = type;
- if (end == NULL)
- end = &start; /* Point to "end" of dll chain. */
- end->next = d; /* Standard linked list stuff. */
- d->next = NULL;
- d->prev = end;
- end = d;
- tot++;
+ append (d);
if (type == DLL_LOAD)
loaded_dlls++;
}
@@ -168,6 +179,119 @@ dll_list::alloc (HINSTANCE h, per_proces
return d;
}
+void
+dll_list::append (dll* d)
+{
+ if (end == NULL)
+end = &start; /* Point to "end" of dll chain. */
+ end->next = d; /* Standard linked list stuff. */
+ d->next = NULL;
+ d->prev = end;
+ end = d;
+ tot++;
+}
+
+void dll_list::populate_deps (dll* d)
+{
+ WCHAR wmodname[NT_MAX_PATH];
+ pefile* pef = (pefile*) d->handle;
+ PIMAGE_DATA_DIRECTORY dd = pef->idata_dir (IMAGE_DIRECTORY_ENTRY_IMPORT);
+ /* Annoyance: calling crealloc with a NULL pointer will use the
+ wrong heap and crash, so we have to replicate some code */
+ long maxdeps = 4;
+ d->deps = (dll**) cmalloc (HEAP_2_DLL, maxdeps*sizeof (dll*));
+ d->ndeps = 0;
+ for (PIMAGE_IMPORT_DESCRIPTOR id=
+ (PIMAGE_IMPORT_DESCRIPTOR) pef->rva (dd->VirtualAddress);
+ dd->Size && id->Name;
+ id++)
+{
+ char* modname = pef->rva (id->Name);
+ sys_mbstowcs (wmodname, NT_MAX_PATH, modname);
+ if (dll* dep = find_by_modname (wmodname))
+ {
+ if (d->ndeps >= maxdeps)
+ {
+ maxdeps = 2*(1+maxdeps);
+ d->deps = (dll**) crealloc (d->deps, maxdeps*sizeof (dll*));
+ }
+ d->deps[d->ndeps++] = dep;
+ }
+}
+
+ /* add one to differentiate no deps from unknown */
+ d->ndeps++;
+}
+
+
+void
+dll_list::topsort ()
+{
+ /* Anything to do? */
+ if (!end)
+return;
+
+ /* make sure we have all the deps available */
+ dll* d = &start;
+ while ((d = d->next))
+if (!d->ndeps)
+ populate_deps (d);
+
+ /* unlink head and tail pointers so the sort can rebuild the list */
+ d = start.next;
+ start.next = end = NULL;
+ topsort_visit (d, true);
+
+ /* clear node markings made by the sort */
+ d = &start;
+ while ((d = d->next))
+{
+ debug_printf ("%W", d->modname);
+ for (int i=1; i < -d->ndeps; i++)
+ debug_printf ("-> %W", d->deps[i-1]->modname);
+
+ /* It would be really nice to be able to keep this information
+around for next time, but we don't have an easy way to
+invalidate cached dependencies when a module unloads. */
+ d->ndeps = 0;
+ cfree (d->deps);
+ d->deps = NULL;
+}
+}
+
+/* A recursive in-place topological sort. The result is ordered so that
+ dependencies of a dll appear before it in the list.
+
+ NOTE: this algorithm is guaranteed to terminate with a "partial
+ order" of dlls but does not do anything smart about cycles: an
+ arbitrary dependent dll will necessarily appear first. Perhaps not
+ surprisingly, Windows ships several dlls containing dependency
+ cycles, including SspiCli/RPCRT4.dll and a lovely tangle involving
+ USP10/LPK/GDI32/USER32.dll). Fortunately, we don't care about
+ Windows DLLs here, and cygwin dlls should behave better */
+void
+dll_list::topsort_visit (dll* d, bool seek_tail)
+{
+ /* Recurse to the end of the dll chain, then visit nodes as we
+ unwind. We do this because once we start visiting nodes we can no
+ longer trust any _next_ pointers.
+
+ We "mark" visited nodes (to avoid revisiting them) by negatin
Improvements to fork handling (3/5)
Hi all,
This patch fixes a bug in the reserve_at function which caused it to
sometimes reserve space needed by the dll it was supposed to help land.
This happens when the dll tries to land in a free region which overlaps
the desired location. The new code exploits the image introspection
(patch #2) to get the dll's image size and avoids the corner cases.
Ryan
diff --git a/dll_init.cc b/dll_init.cc
--- a/dll_init.cc
+++ b/dll_init.cc
@@ -164,6 +164,7 @@ dll_list::alloc (HINSTANCE h, per_proces
d->handle = h;
d->has_dtors = true;
d->p = p;
+ d->image_size = ((pefile*)h)->optional_hdr ()->SizeOfImage;
d->ndeps = 0;
d->deps = NULL;
d->modname = wcsrchr (d->name, L'\\');
@@ -407,21 +408,33 @@ release_upto (const PWCHAR name, DWORD h
}
}
-/* Mark one page at "here" as reserved. This may force
- Windows NT to load a DLL elsewhere. */
+/* Reserve the chunk of free address space starting _here_ and (usually)
+ covering at least _dll_size_ bytes. However, we must take care not
+ to clobber the dll's target address range because it often overlaps.
+ */
static DWORD
-reserve_at (const PWCHAR name, DWORD here)
+reserve_at (const PWCHAR name, DWORD here, DWORD dll_base, DWORD dll_size)
{
DWORD size;
MEMORY_BASIC_INFORMATION mb;
if (!VirtualQuery ((void *) here, &mb, sizeof (mb)))
-size = 64 * 1024;
-
+api_fatal ("couldn't examine memory at %08lx while mapping %W, %E",
+ here, name);
if (mb.State != MEM_FREE)
return 0;
size = mb.RegionSize;
+
+ // don't clobber the space where we want the dll to land
+ DWORD end = here + size;
+ DWORD dll_end = dll_base + dll_size;
+ if (dll_base < here && dll_end > here)
+ here = dll_end; // the dll straddles our left edge
+ else if (dll_base >= here && dll_base < end)
+ end = dll_base; // the dll overlaps partly or fully to our right
+
+ size = end - here;
if (!VirtualAlloc ((void *) here, size, MEM_RESERVE, PAGE_NOACCESS))
api_fatal ("couldn't allocate memory %p(%d) for '%W' alignment, %E\n",
here, size, name);
@@ -499,7 +512,8 @@ dll_list::load_after_fork (HANDLE parent
can in the child, due to differences in the load ordering.
Block memory at it's preferred address and try again. */
if ((DWORD) h > (DWORD) d->handle)
-preferred_block = reserve_at (d->name, (DWORD) h);
+preferred_block = reserve_at (d->name, (DWORD) h,
+ (DWORD) d->handle, d->image_size);
}
}
diff --git a/dll_init.h b/dll_init.h
--- a/dll_init.h
+++ b/dll_init.h
@@ -52,6 +52,7 @@ struct dll
int count;
bool has_dtors;
dll_type type;
+ DWORD image_size;
long ndeps;
dll** deps;
PWCHAR modname;
* dll_init.cc (dll_list::alloc): initialize dll::image_size.
(reserve_at): no longer reserves space needed by the target dll if
the latter overlaps the free region to be blocked.
(dll_list::load_after_fork): use new version of reserve_at.
* dll_init.h (struct dll): add new members to track dll size.
Improvements to fork handling (4/5)
Hi all,
This patch rewrites dll_list::load_after fork. The new version
eliminates reserve_upto() and release_upto(), which were expensive (the
process repeats for each dll) and buggy (release_upto could free
allocations reserve_upto did not make). Instead, the effect of
reserve_upto is achieved by recursively attempting to load each dll in
its proper place and calling reserve_at before retrying; each
reservation's location is kept on the stack throughout and release_at
calls are made only when the recursion unwinds after all dlls have
loaded. Further, the code (again exploiting image introspection from
patch #2) pre-reserves all space needed by each DLL_LOAD before starting
the normal load process. This allows us to detect early whether Windows
clobbered something from the start (allowing retry) and also ensures
that the needed address space is not clobbered by later calls to
reserve_at or by dlls allocating resources.
Ryan
diff --git a/dll_init.cc b/dll_init.cc
--- a/dll_init.cc
+++ b/dll_init.cc
@@ -165,6 +165,7 @@ dll_list::alloc (HINSTANCE h, per_proces
d->has_dtors = true;
d->p = p;
d->image_size = ((pefile*)h)->optional_hdr ()->SizeOfImage;
+ d->preferred_base = (void*) ((pefile*)h)->optional_hdr()->ImageBase;
d->ndeps = 0;
d->deps = NULL;
d->modname = wcsrchr (d->name, L'\\');
@@ -357,56 +358,6 @@ dll_list::init ()
#define A64K (64 * 1024)
-/* Mark every memory address up to "here" as reserved. This may force
- Windows NT to load a DLL in the next available, lowest slot. */
-static void
-reserve_upto (const PWCHAR name, DWORD here)
-{
- DWORD size;
- MEMORY_BASIC_INFORMATION mb;
- for (DWORD start = 0x1; start < here; start += size)
-if (!VirtualQuery ((void *) start, &mb, sizeof (mb)))
- size = A64K;
-else
- {
- size = A64K * ((mb.RegionSize + A64K - 1) / A64K);
- start = A64K * (((DWORD) mb.BaseAddress + A64K - 1) / A64K);
-
- if (start + size > here)
- size = here - start;
- if (mb.State == MEM_FREE &&
- !VirtualAlloc ((void *) start, size, MEM_RESERVE, PAGE_NOACCESS))
- api_fatal ("couldn't allocate memory %p(%d) for '%W' alignment, %E\n",
-start, size, name);
- }
-}
-
-/* Release all of the memory previously allocated by "upto" above.
- Note that this may also free otherwise reserved memory. If that becomes
- a problem, we'll have to keep track of the memory that we reserve above. */
-static void
-release_upto (const PWCHAR name, DWORD here)
-{
- DWORD size;
- MEMORY_BASIC_INFORMATION mb;
- for (DWORD start = 0x1; start < here; start += size)
-if (!VirtualQuery ((void *) start, &mb, sizeof (mb)))
- size = 64 * 1024;
-else
- {
- size = mb.RegionSize;
- if (!(mb.State == MEM_RESERVE && mb.AllocationProtect == PAGE_NOACCESS
- && (((void *) start < cygheap->user_heap.base
-|| (void *) start > cygheap->user_heap.top)
-&& ((void *) start < (void *) cygheap
-|| (void *) start
- > (void *) ((char *) cygheap + CYGHEAPSIZE)
- continue;
- if (!VirtualFree ((void *) start, 0, MEM_RELEASE))
- api_fatal ("couldn't release memory %p(%d) for '%W' alignment, %E\n",
-start, size, name);
- }
-}
/* Reserve the chunk of free address space starting _here_ and (usually)
covering at least _dll_size_ bytes. However, we must take care not
@@ -450,72 +401,130 @@ release_at (const PWCHAR name, DWORD her
here, name);
}
+/* Step 1: Reserve memory for all DLL_LOAD dlls. This is to prevent
+ anything else from taking their spot as we compensate for Windows
+ randomly relocating things.
+
+ NOTE: because we can't depend on LoadLibraryExW to do the right
+ thing, we have to do a vanilla VirtualAlloc instead. One possible
+ optimization might attempt a LoadLibraryExW first, in case it lands
+ in the right place, but then we have to find a way of tracking
+ which dlls ended up needing VirtualAlloc after all. */
+void
+dll_list::reserve_space ()
+{
+ for (dll* d = dlls.istart (DLL_LOAD); d; d = dlls.inext ())
+if (!VirtualAlloc (d->handle, d->image_size, MEM_RESERVE, PAGE_NOACCESS))
+ fork_api_fatal ("Address space needed by '%W' (%08lx) is already
occupied",
+ d->modname, d->handle);
+}
+
/* Reload DLLs after a fork. Iterates over the list of dynamically loaded
DLLs and attempts to load them in the same place as they were loaded in the
parent. */
void
dll_list::load_after_fork (HANDLE parent)
{
- DWORD preferred_block = 0;
+ // moved to frok::child for performance reasons:
+ // dll_list::reserve_space();
+
+ load_after_fork_impl (parent, dlls.istart (DLL_LOAD), 0);
+}
- for (dll *d = &dlls.start; (d = d->next) != NULL; )
-if (d->type == DLL_LOAD)
- for (int i = 0; i < 2; i++)
+static i
Improvements to fork handling (5/5)
Hi all,
This last patch adds a small optimization which reserves the lower 4MB
of address space early in the process's lifetime (even if it's not a
forkee). This was motivated by the observation that Windows tends to
move things around a lot in that area, increasing the probability of
future fork failures if the parent allows cygwin dlls to land there.
The patch does not fully address the problem, however, because ASLR
clobbers addresses above 4M as well. As a result, this patch may or may
not improve fork success rates in practice: most fork failures for me
involve DLL_LINK dlls which landed badly in the child.
It should be independent of the other patches.
Ryan
diff --git a/dcrt0.cc b/dcrt0.cc
--- a/dcrt0.cc
+++ b/dcrt0.cc
@@ -792,6 +792,13 @@ dll_crt0_1 (void *)
main_vfork = vfork_storage.create ();
#endif
+ /* Windows doesn't use the lower 4MB of address space consistently,
+ and those uses arise before cygwin1.dll loads. If a dll loads
+ there we risk being unable to fork later. To avoid the problem,
+ we just reserve everything that's left in that space -- windows
+ can still do what it wants since it got there first. */
+ dlls.block_bad_address_space ();
+
cygbench ("pre-forkee");
if (in_forkee)
{
diff --git a/dll_init.cc b/dll_init.cc
--- a/dll_init.cc
+++ b/dll_init.cc
@@ -359,6 +359,44 @@ dll_list::init ()
#define A64K (64 * 1024)
+void
+dll_list::block_bad_address_space ()
+{
+ /* For some reason VirtualQuery doesn't return consistent values of
+ RegionSize for free space, so we have to compute it manually by
+ looking for MEM_FREE followed by a not-free region. We ensure not
+ to leave a danging free region by allowing the loop to examine
+ 0x0040, which is always the address of the application's
+ executable image.
+ */
+ MEMORY_BASIC_INFORMATION mb;
+ DWORD here;
+ for (DWORD i=A64K; i <= 64*A64K; i += mb.RegionSize)
+{
+ if ( !VirtualQuery ((void*)i, &mb, sizeof(mb)))
+ api_fatal ("-> unable to examine address space at %08lx, %E", i);
+ here = (DWORD) mb.BaseAddress;
+
+ /* this should never happen. If it does we'll need to write some
+code to compensate for it */
+ if (here != i)
+ api_fatal ("VirtualQuery returned info for %lx instead of %lx",
+ here, i);
+ if (mb.State == MEM_FREE)
+ {
+ DWORD size = mb.RegionSize;
+ if (!VirtualAlloc ((void*) here, size, MEM_RESERVE, PAGE_NOACCESS))
+ system_printf ("-> couldn't block out %08lx, %E", here);
+ }
+ else if (mb.RegionSize & (A64K-1))
+ {
+ /* skip free space at the end of mapped slices -- they can't
+be used by anything else */
+ mb.RegionSize = (mb.RegionSize + A64K - 1) & -A64K;
+ }
+}
+}
+
/* Reserve the chunk of free address space starting _here_ and (usually)
covering at least _dll_size_ bytes. However, we must take care not
to clobber the dll's target address range because it often overlaps.
diff --git a/dll_init.h b/dll_init.h
--- a/dll_init.h
+++ b/dll_init.h
@@ -83,6 +83,7 @@ public:
int tot;
int loaded_dlls;
int reload_on_fork;
+ void block_bad_address_space ();
dll *operator [] (const PWCHAR name);
dll *alloc (HINSTANCE, per_process *, dll_type);
dll *find (void *);
* dcrt0.cc (dll_crt0_1): call dll_list::block_bad_address_space
* dll_init.cc (dll_list::block_bad_address_space): new function to
reserve all free space in the low 4MB. Reduces somewhat the
probability of a dynamic dll clashing with windows heaps or thread
stacks.
* dll_init.h (struct dll_list): declaration for above.
Re: Improvements to fork handling
On 11/05/2011 12:16 PM, Christopher Faylor wrote: On Wed, May 11, 2011 at 10:21:14AM -0400, Ryan Johnson wrote: On 11/05/2011 10:13 AM, Christopher Faylor wrote: On Wed, May 11, 2011 at 09:59:53AM +0200, Corinna Vinschen wrote: On May 11 02:18, Ryan Johnson wrote: Please find attached five patches [...] Oops, wrong mailing list... Btw., it would be nice if you could create patches with the diff -p flag as well. It's not exactly essential, but IMHO it's quite a help when trying to review patches. Another problem is this: While you provide separate patches, you don't provide separate ChangeLogs. That makes it kind of hard to apply them separately. Would you mind to create one ChangeLog per change? Ditto. This really needs to be broken down into easier to review chunks. All right. Let's try this again with the correct mailing list. The patches have been generated with diff -p, and each includes appropriate changelog entries. Hopefully the changes are split up finely enough because I don't know a good way to break them down any further. For posterity's sake I'm including the original message body below. Please: One patch per message. One ChangeLog entry per message, not sent as a diff. Done. Please disregard the older (bundled) versions because I found a couple of glitches in the patches which the the 1-per-email set fixes. Ryan
Re: Extending /proc/*/maps
On May 11 13:46, Ryan Johnson wrote:
> On 11/05/2011 7:14 AM, Corinna Vinschen wrote:
> >On May 11 01:27, Ryan Johnson wrote:
> >>The second (proc-maps-heaps) adds reporting of Windows heaps (or
> >>their bases, at least). Unfortunately there doesn't seem to be any
> >>efficient way to identify all virtual allocations which a heap owns.
> >There's a call RtlQueryDebugInformation which can fetch detailed heap
> >information, and which is used by Heap32First/Heap32Last. Using it
> >directly is much more efficient than using the Heap32 functions. The
> >DEBUG_HEAP_INFORMATION is already in ntdll.h, what's missing is the
> >layout of the Blocks info. I found the info by googling:
> >
> > typedef struct _HEAP_BLOCK
> > {
> > PVOID addr;
> > ULONG size;
> > ULONG flags;
> > ULONG unknown;
> > } HEAP_BLOCK, *PHEAP_BLOCK;
> >
> >If this information is searched until the address falls into the just
> >inspected block of virtual memory, then we would have the information,
> >isn't it?
> The problem is that the reported heap blocks corresponded to
> individual malloc() calls when I tested it (unordered list, most
> sizes < 100B). I really would have preferred a function that returns
> the list of memory regions owned by the heap. They must track that
> information -- I highly HeapDestroy uses these heap blocks to decide
> which memory regions to VirtualFree, but that info seems to live in
> the kernel...
No, it doesn't. The heap allocation functions are implemented entirely
in user space. Only virtual memory and shared sections are maintained
by the kernel.
Looking into the MSDN man page for HeapCreate, I'm wondering if this is
really complicated. HeapCreate just allocates a block of memory using
VirtualAlloc. Either the Heap is fixed size, then all allocations are
within the given heap memory area, or the heap can grow, then subsequent
(or big-block) allocations can result in another VirtualAlloc.
Having said that, I don't know where this information is stored exactly,
but it must be available in the DEBUG_HEAP_INFORMATION block. I guess
a bit of experimenting is asked for.
> Given that Heap32* has already been reverse-engineered by others,
> the main challenge would involve sorting the set of heap block
> addresses and distilling them down to a set of allocation bases. We
> don't want to do repeated linear searches over 50k+ heap blocks.
While the base address of the heap is available in
DEBUG_HEAP_INFORMATION, I don't see the size of the heap. Maybe it's in
the block of 7 ULONGs marked as "Reserved"? It must be somewhere.
Assuming just that, you could scan the list of blocks once and drop
those within the orignal heap allocation. The remaining blocks are big
blocks which have been allocated by additional calls to VirtualAlloc.
However, there's a good chance that the entire information about
blocks allocated with VirtualAlloc is kept in the heap's own
datastructures which, again, are undocumented. I would assume the
start of that heap info is at the heaps base address, but without
more information about the internal structure...
> Also, the cygheap isn't a normal windows heap, is it? I thought it
> was essentially a statically-allocated array (.cygheap) that gets
> managed as a heap. I guess since it's part of cygwin1.dll we already
> do sort of report it...
The cygheap is the last section in the DLL and gets allocated by the
Windows loader. The memory management is entirely in Cygwin (cygheap.cc).
Cygwin can raise the size of the cygheap, but only if the blocks right
after the existing cygheap are not already allocated.
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Project Co-Leader cygwin AT cygwin DOT com
Red Hat
Re: [PATCH] Fix /proc/meminfo and /proc/swaps for >4GB
On Mon, 2011-05-09 at 09:55 +0200, Corinna Vinschen wrote: > I'm not sure I understand this new format. Why do you keep the Mem: and > Swap: lines? Linux doesn't have them and top appears to work without > them. And then, why do you print MemShared, HighTotal, and HighFree, > even though they are always 0, but not all the other ~40 lines Linux' > meminfo has, too? Actually, my patch makes no attempt to change the actual format of /proc/meminfo; it changes only what is necessary to handle RAM or swap larger than 4GB by using ULLs instead of ULs. As for modernizing/fixing the format, true, the Mem: and Swap: lines do not exist in modern Linux, nor does the MemShared: line. I would like to actually define at least HighTotal and HighFree; I'll try to look into that further soon. As for the rest of Linux's /proc/meminfo, I'll have to see how many other lines can be reasonably determined (if they would exist at all) on Windows. So with the ULL changes, if I remove the Mem, Swap, and MemShared lines, will that do for now? Yaakov
Re: [PATCH] Fix /proc/meminfo and /proc/swaps for >4GB
On May 11 22:58, Yaakov (Cygwin/X) wrote: > On Mon, 2011-05-09 at 09:55 +0200, Corinna Vinschen wrote: > > I'm not sure I understand this new format. Why do you keep the Mem: and > > Swap: lines? Linux doesn't have them and top appears to work without > > them. And then, why do you print MemShared, HighTotal, and HighFree, > > even though they are always 0, but not all the other ~40 lines Linux' > > meminfo has, too? > > Actually, my patch makes no attempt to change the actual format > of /proc/meminfo; it changes only what is necessary to handle RAM or > swap larger than 4GB by using ULLs instead of ULs. That's fine and thanks for that. I'm only puzzled what's actually printed. > As for modernizing/fixing the format, true, the Mem: and Swap: lines do > not exist in modern Linux, nor does the MemShared: line. I would like > to actually define at least HighTotal and HighFree; I'll try to look > into that further soon. As for the rest of Linux's /proc/meminfo, I'll > have to see how many other lines can be reasonably determined (if they > would exist at all) on Windows. That would be cool, but that wasn't what I meant. I was just puzzled that you added values which are always 0, but left out others which are always 0, too. I was missing a system, kind of. > So with the ULL changes, if I remove the Mem, Swap, and MemShared lines, > will that do for now? Sure. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
