Bug in cmalloc? Was: Re: Problems with: Improvements to fork handling (2/5)

[Ryan Johnson]

On 30/05/2011 2:24 AM, Christopher Faylor wrote:
> On Sun, May 29, 2011 at 12:27:45PM -0400, Christopher Faylor wrote:
>> On Sun, May 29, 2011 at 01:51:35AM -0400, Ryan Johnson wrote:
>>> So, I defined this small function:
>>>
>>> static void break_cmalloc(int depth, int maxdepth) {
>>>      void* x = cmalloc (HEAP_2_DLL, 32);
>>>      cfree(x);
>>>      if (depth<  maxdepth)
>>>          break_cmalloc(depth+1, maxdepth);
>>> }
>>>
>>> and called it during fork instead of dlls.topsort(), with maxdepth=5. No
>>> bug (as expected).
>>>
>>> Then I moved the call to cfree below the recursion, so memory gets freed
>>> in reverse order. Bang. Bash goes down and takes mintty with it after
>>> briefly flashing 'bad address.' Calling bash from cmd.exe hangs the
>>> latter so badly Windows can't kill it (I guess I'll have to reboot).
>> Thanks for the test case.  I'll investigate later today.
> As it turns out, this is not a bug in cmalloc.  fork() was not designed
> to allow calling cmalloc() after the "frok grouped" definition at the
> beginning of the function.  That records the bounds of the cygheap which
> is passed to the child.  If you call cmalloc and friends after that then
> the child process will never know that the heap has been extended.  Then
> when the heap is copied from the parent by cygheap_fixup_in_child(),
> you'll likely be missing pieces of the parent's cygheap and pieces of
> the freed pool will end up pointing into blocks of memory which are not
> properly initialized.
Ouch... and those pieces that didn't get copied are exactly the ones the 
child tries to read first when loading dlls.

Sorry for the false alarm -- I always assumed that was done after the 
call to setjmp, on the parent's side. Should have checked.

BTW, while poring over the patch I noticed a couple irregularities which 
you might want to remove:

1. The declaration for populate_all_deps in dll_init.h is never defined 
or used
2. The incrementing of the (seemingly dead) variable 'tot' ended up in 
dll_list::append and should be moved back to dll_list::alloc where it 
belongs.

Ryan