Re: [PATCH] malloc: Run fork handler as late as possible [BZ #19431]

[Florian Weimer <fweimer at redhat dot com>]

On 02/15/2016 07:10 PM, Torvald Riegel wrote:

>> It explicitly encodes the lock order in the implementations of fork,
>> and does not rely on the registration order, thus avoiding the deadlock.
>>
>> I couldn't test the Hurd bits, but the changes look straightforward enough.
> 
> Are those changes, and thus the new scheme, documented anywhere?

Fork handlers used by the implementation should be invisible to
applications.  The old one wasn't, and this was a bug.

> I think it's really worthwhile to always update and improve the
> documentation, especially if we're kind of (re)discovering knowledge
> about the current implementation such as in this case.  I know this is
> extra work, but if this isn't documented, it's less likely anybody but
> you will be aware of how this all works.

Do you mean something like this inside the fork implementation?

  /* Acquire malloc locks.  This needs to come last because fork
     handlers may use malloc, and the libio list lock has an indirect
     malloc dependency as well (via the getdelim function).  */
  __malloc_fork_lock_parent ();

>> diff --git a/malloc/bug19431.c b/malloc/bug19431.c
>> new file mode 100644
>> index 0000000..3bd876b
>> --- /dev/null
>> +++ b/malloc/bug19431.c
> 
> I haven't seen this naming scheme before. Could you put a tst- prefix in
> there, or something like that?

Okay, I'm going to do that.  GCC uses names of the form âpr33880.câ,
maybe that's where I got the idea.

>> diff --git a/malloc/malloc-private.h b/malloc/malloc-private.h
>> new file mode 100644
>> index 0000000..2fef840
>> --- /dev/null
>> +++ b/malloc/malloc-private.h
> 
> Should this perhaps be malloc-internal.h to match libc-internal.h?

We also have libio/libioP.h, and include/malloc.h.  But I think a
separate header makes sense if its contents is truly internal.

What are the preferences here?

(I hope we will eventually stop build test cases against the internal
headers in include/.)

Florian