Re: [PATCH] drop the deprecated malloc/free hooks in hurd/mach-defpager

[Justus Winter]

Hey :)

Quoting Thomas Schwinge (2016-05-16 18:49:47)
> Hi!
> 
> On Tue, 29 Dec 2015 23:09:54 +0100, Flavio Cruz <flaviocruz@gmail.com> wrote:
> > mach-defpager: Drop the deprecated malloc/free hooks.
> 
> (After approval by Samuel, this became commit
> 8c49801c8f7e3f800cabedf8fca8ccec3cf35a22.)
> 
> > * mach-defpager/kalloc.c: Define malloc and free directly since those are 
> > weak
> > symbols.
> 
> The idea here is to just change the mechanism (override glibc's weak
> symbols instead of using glibc's deprecated malloc hooks), while not
> changing any semantics, right?
> 
> From a quick look, I have some doubts about this generally (so, not only
> related to your change).
> 
> > --- a/mach-defpager/kalloc.c
> > +++ b/mach-defpager/kalloc.c
> 
> > -static void *
> > -malloc_hook (size_t size, const void *caller)
> > +void *
> > +malloc (size_t size)
> >  {
> >    return (void *) kalloc ((vm_size_t) size);
> >  }
> >  
> > -static void
> > -free_hook (void *ptr, const void *caller)
> > +void
> > +free (void *ptr)
> >  {
> >    /* Just ignore harmless attempts at cleanliness.  */
> >    /* panic("free not implemented"); */

Uh, we actually don't free memory here...

> So, here we now only provide the malloc and free symbols.  What happens
> if, for example, calloc, realloc, posix_memalign et al. are called?
> 
> For example, looking at [glibc]/malloc/malloc.c:__libc_calloc (weak alias
> for calloc).  Before your change, it called mach-defpager's malloc_hook,
> and then zeroed that region.  After your change, it is implemented using
> the real glibc calloc -- which actually does change the semantics in an
> unintended way?
> 
> Backing up one step: what is actually being done here in mach-defpager?
> Again from just a quick look, is the idea to use wired memory for all of
> mach-defpager's process memory?
> 
> So, for example, if now some mach-defpager code calls calloc, it will now
> get non-wired "glibc" memory instead of wired "kalloc" memory.  The
> problem here is not mach-defpager's own code (which we can easily
> verify/control), but the problems is the libraries it links against,
> which currently are: libihash, libpthread, glibc itself, and any

And indeed libihash uses calloc.

> dependencies these may have.  Can we be sure these are not internally
> using calloc, for example?
> 
> Now, this is not a new problem that you introduced ;-) -- previously we
> also didn't provide malloc's memalign and realloc hooks, and realloc is
> used quite commonly, I suppose?
> 
> If my theories are correct, do we need to use a different mechanism to
> get the whole mach-defpager process into wired memory?

As Richard said in #hurd, implement mlockall and MCL_FUTURE and just
use the default allocator.

I'd suggest reverting that change, or is the removal of that
deprecated interface imminent?

Cheers,
Justus