Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Jon TURNEY]

On 05/04/2011 17:21, Christopher Faylor wrote:
 On Tue, Apr 05, 2011 at 05:03:43PM +0100, Jon TURNEY wrote:
 On 04/04/2011 15:39, Christopher Faylor wrote:
 I'm trying to imagine a scenario where it would screw up to just do the
 reserve_upto + reserve the low block and I can't think of one.  It's
 potentially a little more work, of course, but I think it may catch the
 more common failing conditions so it shouldn't be too noticeable.
 
 If so, it seems like we're allocating and freeing the space up to the DLL 
 more
 than once.  I think we could avoid doing that.

 For performance reasons, I think you are right.  Or do you mean there is a
 correctness issue with that?

 If you indicate your preferences I'll respin the patch.

 1) Combine passes 2 and 3
 
 I'd prefer this.  If we can get people test the snapshot maybe we an
 figure out if a separate loop is useful.

Updated patch attached.
2011-04-06  Jon TURNEY  jon.tur...@dronecode.org.uk

* dll_init.cc (reserve_at, release_at): New functions.
(load_after_fork): If the DLL was loaded higher than the required
address, assume that it loaded at it's base address and also reserve
memory there to force it to be relocated.

Index: cygwin/dll_init.cc
===
--- cygwin/dll_init.cc.orig 2011-04-06 10:32:08.242187500 +0100
+++ cygwin/dll_init.cc  2011-04-06 10:44:33.25000 +0100
@@ -257,12 +257,44 @@
   }
 }
 
+/* Mark one page at here as reserved.  This may force
+   Windows NT to load a DLL elsewhere. */
+static DWORD
+reserve_at (const PWCHAR name, DWORD here)
+{
+  DWORD size;
+  MEMORY_BASIC_INFORMATION mb;
+
+  if (!VirtualQuery ((void *) here, mb, sizeof (mb)))
+size = 64 * 1024;
+
+  if (mb.State != MEM_FREE)
+return 0;
+
+  size = mb.RegionSize;
+  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);
+  return here;
+}
+
+/* Release the memory previously allocated by reserve_at above. */
+static void
+release_at (const PWCHAR name, DWORD here)
+{
+  if (!VirtualFree ((void *) here, 0, MEM_RELEASE))
+api_fatal (couldn't release memory %p for '%W' alignment, %E\n,
+   here, name);
+}
+
 /* 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;
+
   for (dll *d = dlls.start; (d = d-next) != NULL; )
 if (d-type == DLL_LOAD)
   for (int i = 0; i  2; i++)
@@ -284,10 +316,18 @@
  if (h == d-handle)
h = LoadLibraryW (d-name);
}
+
  /* If we reached here on the second iteration of the for loop
 then there is a lot of memory to release. */
  if (i  0)
-   release_upto (d-name, (DWORD) d-handle);
+{
+  release_upto (d-name, (DWORD) d-handle);
+
+  if (preferred_block)
+release_at(d-name, preferred_block);
+  preferred_block = 0;
+}
+
  if (h == d-handle)
break;  /* Success */
 
@@ -297,11 +337,20 @@
   d-name, d-handle, h);
 
  /* Dll loaded in the wrong place.  Dunno why this happens but it
-always seems to happen when there are multiple DLLs attempting to
-load into the same address space.  In the forked process, the
-second DLL always loads into a different location. So, block all
-of the memory up to the new load address and try again. */
+ always seems to happen when there are multiple DLLs with the
+ same base address.  In the forked process, the relocated DLL
+ may load at a different address. So, block all of the memory up
+ to the relocated load address and try again. */
  reserve_upto (d-name, (DWORD) d-handle);
+
+  /* Also, if the DLL loaded at a higher address than wanted (probably
+ it's base address), reserve the memory at that address. This can
+ happen if it couldn't load at the preferred base in the parent, 
but
+ 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);
+
}
   in_forkee = false;
 }

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Christopher Faylor]

On Wed, Apr 06, 2011 at 12:54:42PM +0100, Jon TURNEY wrote:
On 05/04/2011 17:21, Christopher Faylor wrote:
 On Tue, Apr 05, 2011 at 05:03:43PM +0100, Jon TURNEY wrote:
 On 04/04/2011 15:39, Christopher Faylor wrote:
 I'm trying to imagine a scenario where it would screw up to just do the
 reserve_upto + reserve the low block and I can't think of one.  It's
 potentially a little more work, of course, but I think it may catch the
 more common failing conditions so it shouldn't be too noticeable.
 
 If so, it seems like we're allocating and freeing the space up to the DLL 
 more
 than once.  I think we could avoid doing that.

 For performance reasons, I think you are right.  Or do you mean there is a
 correctness issue with that?

 If you indicate your preferences I'll respin the patch.

 1) Combine passes 2 and 3
 
 I'd prefer this.  If we can get people test the snapshot maybe we an
 figure out if a separate loop is useful.

Updated patch attached.

2011-04-06  Jon TURNEY  jon.tur...@dronecode.org.uk

   * dll_init.cc (reserve_at, release_at): New functions.
   (load_after_fork): If the DLL was loaded higher than the required
   address, assume that it loaded at it's base address and also reserve
   memory there to force it to be relocated.

This looks good except for formatting nits involving spaces before/after
parentheses.  I've checked it in with those very minor changes.

Thanks very much for this patch.

cgf

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Christopher Faylor]

On Tue, Apr 05, 2011 at 05:03:43PM +0100, Jon TURNEY wrote:
On 04/04/2011 15:39, Christopher Faylor wrote:
 On Mon, Apr 04, 2011 at 01:42:54PM +0100, Jon TURNEY wrote:
 Attached is an updated version of the patch which fixes the warning 
 identified
 by Yaakov.

 I've also attached a slightly cleaned up version of the additional fork
 debugging output patch I was using.
 

 2011-03-12  Jon TURNEY  jon.tur...@dronecode.org.uk

 * dll_init.cc (reserve_at, release_at): New functions.
 (load_after_fork): Make a 3rd pass at trying to load the DLL in
 the right place.
 
 Rather than add a new pass could we just add rename/enhance reserve_upto so
 that it both reserves the block of memory up to the dll's preferred load 
 address
 and the block of memory erroneously occupied by the dll?  Or is the extra 
 step
 important?

I don't know if the 2nd and 3rd passes can be combined.

From observation, the behaviour of LoadLibraryEx() seems to be to load the DLL
at it's base address if it can, otherwise the lowest available address.  If
that's accurate then there doesn't seem to be any harm in combining them
(as pass 3 will always succeed in successfully remapping the DLL if pass 2
would have), but that's based on my limited observations on a single Windows
version.

I guess I'm just being conservative to avoid the possibility of a regression
if there are circumstances I don't know about where pass 2 would succeed but
pass 3 would fail.

I'm trying to imagine a scenario where it would screw up to just do the
reserve_upto + reserve the low block and I can't think of one.  It's
potentially a little more work, of course, but I think it may catch the
more common failing conditions so it shouldn't be too noticeable.

 If so, it seems like we're allocating and freeing the space up to the DLL 
 more
 than once.  I think we could avoid doing that.

For performance reasons, I think you are right.  Or do you mean there is a
correctness issue with that?

If you indicate your preferences I'll respin the patch.

1) Combine passes 2 and 3

I'd prefer this.  If we can get people test the snapshot maybe we an
figure out if a separate loop is useful.

cgf

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Jon TURNEY]

On 30/03/2011 22:29, Christopher Faylor wrote:
 On Wed, Mar 30, 2011 at 11:15:56PM +0200, Corinna Vinschen wrote:
 Chris, are you going to take a look into this patch?
 
 yep.

Attached is an updated version of the patch which fixes the warning identified
by Yaakov.

I've also attached a slightly cleaned up version of the additional fork
debugging output patch I was using.

2011-03-12  Jon TURNEY  jon.tur...@dronecode.org.uk

* dll_init.cc (reserve_at, release_at): New functions.
(load_after_fork): Make a 3rd pass at trying to load the DLL in
the right place.


Index: cygwin/dll_init.cc
===
--- cygwin/dll_init.cc.orig 2011-03-27 18:19:06.0 +0100
+++ cygwin/dll_init.cc  2011-03-28 23:43:47.12500 +0100
@@ -254,15 +254,47 @@
   }
 }
 
+/* Mark one page at here as reserved.  This may force
+   Windows NT to load a DLL elsewhere. */
+static DWORD
+reserve_at (const PWCHAR name, DWORD here)
+{
+  DWORD size;
+  MEMORY_BASIC_INFORMATION mb;
+
+  if (!VirtualQuery ((void *) here, mb, sizeof (mb)))
+size = 64 * 1024;
+
+  if (mb.State != MEM_FREE)
+return 0;
+
+  size = mb.RegionSize;
+  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);
+  return here;
+}
+
+/* Release the memory previously allocated by reserve_at above. */
+static void
+release_at (const PWCHAR name, DWORD here)
+{
+  if (!VirtualFree ((void *) here, 0, MEM_RELEASE))
+api_fatal (couldn't release memory %p for '%W' alignment, %E\n,
+   here, name);
+}
+
 /* 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;
+
   for (dll *d = dlls.start; (d = d-next) != NULL; )
 if (d-type == DLL_LOAD)
-  for (int i = 0; i  2; i++)
+  for (int i = 0; i  3; i++)
{
  /* See if DLL will load in proper place.  If so, free it and reload
 it the right way.
@@ -281,15 +313,26 @@
  if (h == d-handle)
h = LoadLibraryW (d-name);
}
- /* If we reached here on the second iteration of the for loop
+
+ /* If we reached here on subsequent iterations of the for loop
 then there is a lot of memory to release. */
  if (i  0)
release_upto (d-name, (DWORD) d-handle);
+
+  /* If we reached here on the last iteration of the for loop
+ then there's a bit of memory to release */
+  if (i  1)
+{
+  if (preferred_block)
+release_at(d-name, preferred_block);
+  preferred_block = 0;
+}
+
  if (h == d-handle)
break;  /* Success */
 
- if (i  0)
-   /* We tried once to relocate the dll and it failed. */
+ if (i  1)
+   /* We tried to relocate the dll and it failed. */
api_fatal (unable to remap %W to same address as parent: %p != %p,
   d-name, d-handle, h);
 
@@ -299,6 +342,15 @@
 second DLL always loads into a different location. So, block all
 of the memory up to the new load address and try again. */
  reserve_upto (d-name, (DWORD) d-handle);
+
+  /* Dll *still* loaded in the wrong place.  This can happen if it
+ couldn't load at the preferred base in the parent, but can in
+ the child, due to ordering differences.  Block memory at it's
+ preferred address and try again. */
+  if (i  0)
+{
+  preferred_block = reserve_at(d-name, (DWORD)h);
+}
}
   in_forkee = false;
 }
2011-04-04  Jon TURNEY  jon.tur...@dronecode.org.uk

* dll_init.cc (load_after_fork): Add some debug output...
* environ.cc (parse_thing): ... controlled by 'forkdebug' setting


Index: winsup/cygwin/dll_init.cc
===
--- winsup.orig/cygwin/dll_init.cc  2011-04-02 21:40:02.0 +0100
+++ winsup/cygwin/dll_init.cc   2011-04-03 18:47:09.78125 +0100
@@ -29,6 +29,7 @@
 dll_list dlls;
 
 static bool dll_global_dtors_recorded;
+bool fork_debug = FALSE;
 
 /* Run destructors for all DLLs on exit. */
 void
@@ -307,6 +308,8 @@
 above, the second LoadLibrary will not execute its startup code
 unless it is first unloaded. */
  HMODULE h = LoadLibraryExW (d-name, NULL, 
DONT_RESOLVE_DLL_REFERENCES);
+ if (fork_debug)
+   system_printf(LoadLibrary %W @ %p (pass %d), d-name, h, i);
 
  if (!h)
system_printf (can't reload %W, %E, d-name);
@@ -332,7 +335,11 @@
 }
 
  if (h == 

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Christopher Faylor]

On Mon, Apr 04, 2011 at 01:42:54PM +0100, Jon TURNEY wrote:
On 30/03/2011 22:29, Christopher Faylor wrote:
 On Wed, Mar 30, 2011 at 11:15:56PM +0200, Corinna Vinschen wrote:
 Chris, are you going to take a look into this patch?
 
 yep.

Attached is an updated version of the patch which fixes the warning identified
by Yaakov.

I've also attached a slightly cleaned up version of the additional fork
debugging output patch I was using.


2011-03-12  Jon TURNEY  jon.tur...@dronecode.org.uk

   * dll_init.cc (reserve_at, release_at): New functions.
   (load_after_fork): Make a 3rd pass at trying to load the DLL in
   the right place.

Rather than add a new pass could we just add rename/enhance reserve_upto so
that it both reserves the block of memory up to the dll's preferred load address
and the block of memory erroneously occupied by the dll?  Or is the extra step
important?

If so, it seems like we're allocating and freeing the space up to the DLL more
than once.  I think we could avoid doing that.

As far as fork debugging is concerned, do you know about building cygwin with
--enable-debugging and then setting CYGWIN_DEBUG=blah?  That will force a
gdb to start whenever cygwin runs a program named blah.  That's how I usually
debug projects like this.

--enable-debugging has some irritating races so you have to use it sparingly but
it's invaluable for figuring out problems in forked processes.

cgf

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Corinna Vinschen]

Hi Jon,

On Mar 15 16:46, Corinna Vinschen wrote:
 On Mar 15 11:04, Christopher Faylor wrote:
  On Tue, Mar 15, 2011 at 08:53:13AM +0100, Corinna Vinschen wrote:
  On Mar 14 22:02, Jon TURNEY wrote:
   On 13/03/2011 15:21, Corinna Vinschen wrote:
Thanks for the patch, but afaics you don't have a copyright assignment
on file with Red Hat.  It's unfortunately required for substantial
patches.  Please see http://cygwin.com/contrib.html, especially the
Before you get started section.
   
   No problem, I have signed and posted an assignment, although I'm not 
   sure I
   consider this patch 'substantial' :-)
  
  Thanks.  I'm looking forward to get it.
  
  I think your patch is a good idea, but apart from the fact that I have
  to wait for your copyright assignment, I'm reluctant to add it to 1.7.9.
  As you probably have seen in CVS, I'm adding new stuff only to a
  post-1.7.9 branch right now.
  
  And, since this is my code, I'd like to have the final approval on whether
  it goes in or not.
 
 Sure.

Your copyright assignment has been ountersigned by my manager today.
Chris, are you going to take a look into this patch?


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Christopher Faylor]

On Wed, Mar 30, 2011 at 11:15:56PM +0200, Corinna Vinschen wrote:
Hi Jon,

On Mar 15 16:46, Corinna Vinschen wrote:
 On Mar 15 11:04, Christopher Faylor wrote:
  On Tue, Mar 15, 2011 at 08:53:13AM +0100, Corinna Vinschen wrote:
  On Mar 14 22:02, Jon TURNEY wrote:
   On 13/03/2011 15:21, Corinna Vinschen wrote:
Thanks for the patch, but afaics you don't have a copyright assignment
on file with Red Hat.  It's unfortunately required for substantial
patches.  Please see http://cygwin.com/contrib.html, especially the
Before you get started section.
   
   No problem, I have signed and posted an assignment, although I'm not 
   sure I
   consider this patch 'substantial' :-)
  
  Thanks.  I'm looking forward to get it.
  
  I think your patch is a good idea, but apart from the fact that I have
  to wait for your copyright assignment, I'm reluctant to add it to 1.7.9.
  As you probably have seen in CVS, I'm adding new stuff only to a
  post-1.7.9 branch right now.
  
  And, since this is my code, I'd like to have the final approval on whether
  it goes in or not.
 
 Sure.

Your copyright assignment has been ountersigned by my manager today.
Chris, are you going to take a look into this patch?

yep.

cgf

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Jon TURNEY]

On 15/03/2011 23:37, Yaakov (Cygwin/X) wrote:
 On Sun, 2011-03-13 at 15:07 +, Jon TURNEY wrote:
 Attached is a patch which avoids a fork failure due to remap error in the
 specific circumstances described in my email [1], by adding an additional 
 pass
 to load_after_fork() which forces the DLL to be relocated by 
 VirtualAlloc()ing
 a block of memory at the load address as well.

 Hopefully it can be seen by inspection that this code doesn't change the
 behaviour of the first two passes, and so will only be changing the behaviour
 in what was an fatal error case before.
 
 This patch causes a warning with GCC 4.5:
 
 cc1plus: warnings being treated as errors
 dll_init.cc: In member function ‘void dll_list::load_after_fork(void*)’:
 dll_init.cc:328:33: error: converting to non-pointer type ‘DWORD’ from
 NULL

Thanks, I guess that should be a 0 instead.

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Corinna Vinschen]

On Mar 14 22:02, Jon TURNEY wrote:
 On 13/03/2011 15:21, Corinna Vinschen wrote:
  Thanks for the patch, but afaics you don't have a copyright assignment
  on file with Red Hat.  It's unfortunately required for substantial
  patches.  Please see http://cygwin.com/contrib.html, especially the
  Before you get started section.
 
 No problem, I have signed and posted an assignment, although I'm not sure I
 consider this patch 'substantial' :-)

Thanks.  I'm looking forward to get it.

I think your patch is a good idea, but apart from the fact that I have
to wait for your copyright assignment, I'm reluctant to add it to 1.7.9.
As you probably have seen in CVS, I'm adding new stuff only to a
post-1.7.9 branch right now.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Christopher Faylor]

On Tue, Mar 15, 2011 at 08:53:13AM +0100, Corinna Vinschen wrote:
On Mar 14 22:02, Jon TURNEY wrote:
 On 13/03/2011 15:21, Corinna Vinschen wrote:
  Thanks for the patch, but afaics you don't have a copyright assignment
  on file with Red Hat.  It's unfortunately required for substantial
  patches.  Please see http://cygwin.com/contrib.html, especially the
  Before you get started section.
 
 No problem, I have signed and posted an assignment, although I'm not sure I
 consider this patch 'substantial' :-)

Thanks.  I'm looking forward to get it.

I think your patch is a good idea, but apart from the fact that I have
to wait for your copyright assignment, I'm reluctant to add it to 1.7.9.
As you probably have seen in CVS, I'm adding new stuff only to a
post-1.7.9 branch right now.

And, since this is my code, I'd like to have the final approval on whether
it goes in or not.

cgf

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Corinna Vinschen]

On Mar 15 11:04, Christopher Faylor wrote:
 On Tue, Mar 15, 2011 at 08:53:13AM +0100, Corinna Vinschen wrote:
 On Mar 14 22:02, Jon TURNEY wrote:
  On 13/03/2011 15:21, Corinna Vinschen wrote:
   Thanks for the patch, but afaics you don't have a copyright assignment
   on file with Red Hat.  It's unfortunately required for substantial
   patches.  Please see http://cygwin.com/contrib.html, especially the
   Before you get started section.
  
  No problem, I have signed and posted an assignment, although I'm not sure I
  consider this patch 'substantial' :-)
 
 Thanks.  I'm looking forward to get it.
 
 I think your patch is a good idea, but apart from the fact that I have
 to wait for your copyright assignment, I'm reluctant to add it to 1.7.9.
 As you probably have seen in CVS, I'm adding new stuff only to a
 post-1.7.9 branch right now.
 
 And, since this is my code, I'd like to have the final approval on whether
 it goes in or not.

Sure.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader  cygwin AT cygwin DOT com
Red Hat

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Yaakov (Cygwin/X)]

On Sun, 2011-03-13 at 15:07 +, Jon TURNEY wrote:
 Attached is a patch which avoids a fork failure due to remap error in the
 specific circumstances described in my email [1], by adding an additional pass
 to load_after_fork() which forces the DLL to be relocated by VirtualAlloc()ing
 a block of memory at the load address as well.
 
 Hopefully it can be seen by inspection that this code doesn't change the
 behaviour of the first two passes, and so will only be changing the behaviour
 in what was an fatal error case before.

This patch causes a warning with GCC 4.5:

cc1plus: warnings being treated as errors
dll_init.cc: In member function ‘void dll_list::load_after_fork(void*)’:
dll_init.cc:328:33: error: converting to non-pointer type ‘DWORD’ from
NULL


Yaakov

Re: [PATCH] Add an additional relocation attempt pass to load_after_fork()

[Jon TURNEY]

On 13/03/2011 15:21, Corinna Vinschen wrote:
 Thanks for the patch, but afaics you don't have a copyright assignment
 on file with Red Hat.  It's unfortunately required for substantial
 patches.  Please see http://cygwin.com/contrib.html, especially the
 Before you get started section.

No problem, I have signed and posted an assignment, although I'm not sure I
consider this patch 'substantial' :-)