Re: [PATCH v8 03/10] Remove __morecore and __default_morecore

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 7/14/21 10:12 PM, Guillaume Morin wrote:
> On 14 Jul 19:43, Siddhesh Poyarekar wrote:
> Probably. But you surely know that most people get glibc from their
> distributions and do not build their own glibc. My main box is running
> Debian stable for example. The next release (bullseye) seems like it'll
> be shipping 2.31 (where it's not deprecated yet, I believe). RHEL8 ships
> with a libhugetlbfs rpm (so users do not need to build it at all unlike
> Debian), which will become useless with this change! Most users will
> never have seen the deprecation warning before they see the actual
> removal.

But that means it won't be useless in RHEL8 or current stable 
Debian/Ubuntu, so there's still time for libhugetlbfs to adapt to the 
change.

> That said, I was personally aware since it was pointed out on a
> libhugetlbfs github issue that it was deprecated in 2.32. I think the
> hope was it would be replaced by something usable, and users would not
> be left with *no* solution (I was also quietly hoping Eric Munson, the
> libhugetlbfs maintainer would be reaching out). But if you remove it, I
> am not quite sure what you expect libhugetlbfs users to do, really.

As is evident, I personally haven't followed libhugetlbfs development 
and have been carrying forward work to remove __morecore that started at 
least a year ago, if not more.  That said though, the deprecation 
warning (especially the fact that the release notes specifically 
mentioned malloc interpositioning as the alternative) ought to have made 
it clear that we weren't thinking of an alternative.

> Well, I do not see why morecore() does not make sense as a plugin. I am
> not seeing any technical argument supporting that it will prevent
> clamping down the implementation provided you document the currently
> acceptable semantics for this interface.

Right now the debug DSO that this patch pulls out only supports the 
debugging features (malloc_check, mcheck, mtrace, old hooks) and they're 
not intended to be linked against, only used as compatibility for 
existing programs.  If __morecore is to move there, we'd have to allow 
linking with the debug DSO, which we don't want to do because it 
multiplies the interfaces we need to support.  We'd basically have two 
malloc interfaces to support then.

> libhugetlbfs users are aware this only works in the main arena. I
> certainly agree this is not ideal and this is definitely something that
> could be fixed as well. But again, keep in mind people have been using
> this for 15+ years.
> 
> Obviously, if you're arguing to extend the glibc hookability so it works
> for all arenas, I am all ears, certainly would help out and would be a
> day one user :-)

Unfortunately not, I'd like the system malloc to be as less hookable as 
possible because the hookability is a security nightmare and it prevents 
a lot of optimizations and hardening.

> Again, you seem to be ignoring the 15 years of people using
> libhugetblfs. It's been tested extensively by users. I certainly agree
> it should get testing in the glibc testsuite, but it's actually trivial
> afaict:
> 
> - morecore() in effect has very simple semantics. It takes a size and
>    returns a pointer to the beginning of the new area. The only thing
>    that differs here from sbrk() is the fact that a new area might not be
>    contiguous with the previous segment.  But that's supported in the
>    malloc code, otherwise libhugetlbfs morecore() would never have worked
>    (that's the MORECORE_CONTIGUOUS handling in the malloc code). That's
>    really _all_ you need to test. And I would presume non contiguous
>    heaps are implemented not just for morecore() sake (not sure though).
>    If that's case, no actual specific testing needs to be done.
> 
> - If you _only_ allow morecore interposition through a DSO (as I was
>    suggesting), you do _not_ even need to support non contiguous heaps
>    (at least for morecore()) since at that point it becomes guaranteed
>    that morecore() will always point to the same implementation. Then
>    there is _nothing_ to test at that point. Proper morecore()
>    implementations can semantically behave exactly like sbrk!
> 
> - A morecore2() hook that takes the old top of the heap as well as the
>    size would also allow libhugetlbfs() to implement a perfectly
>    continuous heap and so would semantically behave like sbrk().  But it
>    does not look like it's being considered here :-)

The direction we want to take is to not have to support all this in the 
interest of simplifying the implementation and make it easier to reason, 
audit and debug.

> Honestly from my PoV, some usual feature was added a while ago. The rest
> of the code was changed without paying attention to the feature (bug
> report ignored for 5 years as well). And you're using this as an
> argument to remove it.... I don't think this is tech debt at all, the
> feature is simply paying the price of being ignored. It does _seem_ to
> me that little effort have been made to see if it was workable at all.

Different developers have been involved in this and I suppose the 
developers who wrote the original implementation may even agree with 
you.  However, technology has changed a lot over the years and many of 
the things that were thought to be useful are now either redundant, 
unwieldy or downright dangerous.  The hooks fall somewhere between the 
last two because in the general case they're quite hard to reason and in 
the simplest case, they're unprotected indirections that have 
historically been used as vulnerability primitives for years.

> To be clear, I am not the libhugetblfs maintainer, just a user (that
> investigated the trimming issue). I think everybody is aware they can
> fork the glibc malloc implementation.  Surely you realize that it's not
> ideal but even feasible for most users.

I understand, I'm sorry it has come to this.  I hope libhugetlbfs 
maintainers can find a way out sooner.

Siddhesh