[v8,03/10] Remove __morecore and __default_morecore

  Make the __morecore and __default_morecore symbols compat-only and
remove their declarations from the API.  Also, include morecore.c
directly into malloc.c; this should ideally get merged into malloc in
a future cleanup.
---
 NEWS              |  5 +++++
 include/stdlib.h  |  3 ---
 malloc/Makefile   |  2 +-
 malloc/arena.c    | 12 ++----------
 malloc/hooks.c    |  2 ++
 malloc/malloc.c   |  7 +++----
 malloc/malloc.h   |  8 --------
 malloc/morecore.c | 34 +++++++++++-----------------------
 8 files changed, 24 insertions(+), 49 deletions(-)
  

FYI, there's at least one use case[1] that is adversely affected by 
__morecore removal.  I have closed it as WONTFIX citing that malloc does 
not always do the right thing with arbitrary morecore (and we don't even 
test it) anyway and it's a net win to remove them, but I mention it here 
too in the interest of a wider discussion.

Guillaume, would you like to elaborate on the use case a bit more so 
that we know exactly what we're dealing with?

Siddhesh

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=20646

On 7/13/21 1:08 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> Make the __morecore and __default_morecore symbols compat-only and
> remove their declarations from the API.  Also, include morecore.c
> directly into malloc.c; this should ideally get merged into malloc in
> a future cleanup.
> ---
>   NEWS              |  5 +++++
>   include/stdlib.h  |  3 ---
>   malloc/Makefile   |  2 +-
>   malloc/arena.c    | 12 ++----------
>   malloc/hooks.c    |  2 ++
>   malloc/malloc.c   |  7 +++----
>   malloc/malloc.h   |  8 --------
>   malloc/morecore.c | 34 +++++++++++-----------------------
>   8 files changed, 24 insertions(+), 49 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 13ffe627da..18d9e65eb2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -113,6 +113,11 @@ Deprecated and removed features, and other changes affecting compatibility:
>     mtrace.  Similar functionality can be achieved by using conditional
>     breakpoints within mtrace functions from within gdb.
>   
> +* The __morecore and __after_morecore_hook malloc hooks and the default
> +  implementation __default_morecore have been removed from the API.  Existing
> +  applications will continue to link against these symbols but the interfaces
> +  no longer have any effect on malloc.
> +
>   Changes to build and runtime requirements:
>   
>   * On Linux, the shm_open, sem_open, and related functions now expect the
> diff --git a/include/stdlib.h b/include/stdlib.h
> index 1f6e1508e4..1c6f70b082 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -306,9 +306,6 @@ libc_hidden_proto (__qfcvt_r)
>   #  define MB_CUR_MAX (_NL_CURRENT_WORD (LC_CTYPE, _NL_CTYPE_MB_CUR_MAX))
>   # endif
>   
> -extern void *__default_morecore (ptrdiff_t) __THROW;
> -libc_hidden_proto (__default_morecore)
> -
>   struct abort_msg_s
>   {
>     unsigned int size;
> diff --git a/malloc/Makefile b/malloc/Makefile
> index d15729569b..020f781b59 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -104,7 +104,7 @@ tests-exclude-mcheck = tst-mallocstate \
>   tests-mcheck = $(filter-out $(tests-exclude-mcheck), $(tests))
>   endif
>   
> -routines = malloc morecore mcheck mtrace obstack reallocarray \
> +routines = malloc mcheck mtrace obstack reallocarray \
>     scratch_buffer_dupfree \
>     scratch_buffer_grow scratch_buffer_grow_preserve \
>     scratch_buffer_set_array_size \
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 991fc21a7e..f1693ed48f 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -274,14 +274,6 @@ next_env_entry (char ***position)
>   #endif
>   
>   
> -#if defined(SHARED) || defined(USE_MTAG)
> -static void *
> -__failing_morecore (ptrdiff_t d)
> -{
> -  return (void *) MORECORE_FAILURE;
> -}
> -#endif
> -
>   #ifdef SHARED
>   extern struct dl_open_hook *_dl_open_hook;
>   libc_hidden_proto (_dl_open_hook);
> @@ -310,7 +302,7 @@ ptmalloc_init (void)
>   	 and that morecore does not support tagged regions, then
>   	 disable it.  */
>         if (__MTAG_SBRK_UNTAGGED)
> -	__morecore = __failing_morecore;
> +	__always_fail_morecore = true;
>   
>         mtag_enabled = true;
>         mtag_mmap_flags = __MTAG_MMAP_FLAGS;
> @@ -323,7 +315,7 @@ ptmalloc_init (void)
>        generic sbrk implementation also enforces this, but it is not
>        used on Hurd.  */
>     if (!__libc_initial)
> -    __morecore = __failing_morecore;
> +    __always_fail_morecore = true;
>   #endif
>   
>     thread_arena = &main_arena;
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 45c91d6502..4aa6dadcff 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -20,6 +20,8 @@
>   #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
>   void weak_variable (*__after_morecore_hook) (void) = NULL;
>   compat_symbol (libc, __after_morecore_hook, __after_morecore_hook, GLIBC_2_0);
> +void *(*__morecore)(ptrdiff_t);
> +compat_symbol (libc, __morecore, __morecore, GLIBC_2_0);
>   #endif
>   
>   /* Hooks for debugging versions.  The initial hooks just call the
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 24e7854a0e..6e8fa9e424 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -384,12 +384,11 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
>   #define TRIM_FASTBINS  0
>   #endif
>   
> -
>   /* Definition for getting more memory from the OS.  */
> -#define MORECORE         (*__morecore)
> +#include "morecore.c"
> +
> +#define MORECORE         (*__glibc_morecore)
>   #define MORECORE_FAILURE 0
> -void * __default_morecore (ptrdiff_t);
> -void *(*__morecore)(ptrdiff_t) = __default_morecore;
>   
>   /* Memory tagging.  */
>   
> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 634b7db868..17ab9ee345 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -76,14 +76,6 @@ extern void *valloc (size_t __size) __THROW __attribute_malloc__
>   extern void *pvalloc (size_t __size) __THROW __attribute_malloc__
>     __wur __attr_dealloc_free;
>   
> -/* Underlying allocation function; successive calls should return
> -   contiguous pieces of memory.  */
> -extern void *(*__morecore) (ptrdiff_t __size) __MALLOC_DEPRECATED;
> -
> -/* Default value of `__morecore'.  */
> -extern void *__default_morecore (ptrdiff_t __size)
> -__THROW __attribute_malloc__  __MALLOC_DEPRECATED;
> -
>   /* SVID2/XPG mallinfo structure */
>   
>   struct mallinfo
> diff --git a/malloc/morecore.c b/malloc/morecore.c
> index 047228779b..8168ef158c 100644
> --- a/malloc/morecore.c
> +++ b/malloc/morecore.c
> @@ -15,39 +15,27 @@
>      License along with the GNU C Library; if not, see
>      <https://www.gnu.org/licenses/>.  */
>   
> -#ifndef _MALLOC_INTERNAL
> -# define _MALLOC_INTERNAL
> -# include <malloc.h>
> -#endif
> -
> -#ifndef __GNU_LIBRARY__
> -# define __sbrk  sbrk
> -#endif
> -
> -#ifdef __GNU_LIBRARY__
> -/* It is best not to declare this and cast its result on foreign operating
> -   systems with potentially hostile include files.  */
> -
> -# include <stddef.h>
> -# include <stdlib.h>
> -extern void *__sbrk (ptrdiff_t increment) __THROW;
> -libc_hidden_proto (__sbrk)
> -#endif
> -
> -#ifndef NULL
> -# define NULL 0
> +#if defined(SHARED) || defined(USE_MTAG)
> +static bool __always_fail_morecore = false;
>   #endif
>   
>   /* Allocate INCREMENT more bytes of data space,
>      and return the start of data space, or NULL on errors.
>      If INCREMENT is negative, shrink data space.  */
>   void *
> -__default_morecore (ptrdiff_t increment)
> +__glibc_morecore (ptrdiff_t increment)
>   {
> +#if defined(SHARED) || defined(USE_MTAG)
> +  if (__always_fail_morecore)
> +    return NULL;
> +#endif
> +
>     void *result = (void *) __sbrk (increment);
>     if (result == (void *) -1)
>       return NULL;
>   
>     return result;
>   }
> -libc_hidden_def (__default_morecore)
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
> +compat_symbol (libc, __glibc_morecore, __default_morecore, GLIBC_2_0);
> +#endif
>
  

On 14 Jul 12:31, Siddhesh Poyarekar wrote:
> FYI, there's at least one use case[1] that is adversely affected by
> __morecore removal.  I have closed it as WONTFIX citing that malloc does not
> always do the right thing with arbitrary morecore (and we don't even test
> it) anyway and it's a net win to remove them, but I mention it here too in
> the interest of a wider discussion.
> 
> Guillaume, would you like to elaborate on the use case a bit more so that we
> know exactly what we're dealing with?
> 
> Siddhesh
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=20646

Hello Siddesh,

I replied on the bug report becuase I had not seen this message.

But basically, this breaks https://github.com/libhugetlbfs/libhugetlbfs/
which is old (started in 2006) and commonly used library.  The library
is plugging its own morecore() implementation to use hugetlb pages to
back the malloc heap.

This is definitely not a win for all libhugetlbfs users. We have no
equivalent solution. You're asking to find us an entire new malloc
implementation if there is one (or write one?). We have no way of keep
using glibc's malloc and hugetlb. I am not even sure there exists
an equivalent replacement.

I understand there is some security concern about malloc hooks but then
why not allow morecore() substitution with a properly
documented/supported interface? Most users already LD_PRELOAD
libhugetlbfs so that would be an easy fix.

You're claiming that it's subtly broken. I'd like to understand how and
why this can't be fixed. Afaik people have been using libhugetlbfs's
morecore() in production for 15+ years without any issue.

The only issue I am aware of is the one I reported about 5 years ago
(along with a reproducer and a patch). This problem can only be reached
if trimming is enabled in the morecore implementation (it's disabled in
libhugetlbfs for this very reason).
  

On 7/14/21 6:24 PM, Guillaume Morin wrote:
> On 14 Jul 12:31, Siddhesh Poyarekar wrote:
> I replied on the bug report becuase I had not seen this message.
> 
> But basically, this breaks https://github.com/libhugetlbfs/libhugetlbfs/
> which is old (started in 2006) and commonly used library.  The library
> is plugging its own morecore() implementation to use hugetlb pages to
> back the malloc heap.

Thanks, AFAICT, libhugetlbfs only uses __morecore and not the rest of 
the interfaces.  The thing is, __morecore and friends have been 
deprecated for a year and building anything with __morecore ought to 
give the deprecation warning.

> This is definitely not a win for all libhugetlbfs users. We have no
> equivalent solution. You're asking to find us an entire new malloc
> implementation if there is one (or write one?). We have no way of keep
> using glibc's malloc and hugetlb. I am not even sure there exists
> an equivalent replacement.
> 
> I understand there is some security concern about malloc hooks but then
> why not allow morecore() substitution with a properly
> documented/supported interface? Most users already LD_PRELOAD
> libhugetlbfs so that would be an easy fix.

I don't think we would like to export a different interface to do the 
same thing because it still doesn't allow us to clamp down further on 
the system malloc implementation.  Besides, the morecore hooking 
interface is severely limited because it only affects the main arena. 
Non-main arenas have their own allocation techniques and it doesn't 
really make sense to have an interface to control just the main arena.

> You're claiming that it's subtly broken. I'd like to understand how and
> why this can't be fixed. Afaik people have been using libhugetlbfs's
> morecore() in production for 15+ years without any issue.
> 
> The only issue I am aware of is the one I reported about 5 years ago
> (along with a reproducer and a patch). This problem can only be reached
> if trimming is enabled in the morecore implementation (it's disabled in
> libhugetlbfs for this very reason).

There is only the one known issue that you reported but that may well be 
because the interface isn't well tested.  There have been many changes 
to malloc over the years and I cannot say for certain that the idea that 
morecore could be anything other than brk has been consistently 
considered.  We don't have tests to verify that either.  Further, 
morecore limits its influence to the main arena and it is possible that 
future changes could break this in ways that we cannot anticipate today.

Basically this is technical debt we've accumulated from the days when 
malloc assumed single threaded programs and was incrementally hacked to 
add in multi-threaded support.  We'd like to clean this up.

The malloc code in glibc is quite general purpose and has little 
dependency on libc internals.  It may well be possible for you to copy 
the source files over into libhugetlbfs to implement the interposer 
within libhugetlbfs that does exactly what you need.  In fact, that may 
even allow you to extend hugetlbfs support to non-main arenas.

Siddhesh
  

On 14 Jul 19:43, Siddhesh Poyarekar wrote:
> > But basically, this breaks https://github.com/libhugetlbfs/libhugetlbfs/
> > which is old (started in 2006) and commonly used library.  The library
> > is plugging its own morecore() implementation to use hugetlb pages to
> > back the malloc heap.
> Thanks, AFAICT, libhugetlbfs only uses __morecore and not the rest of the
> interfaces.  The thing is, __morecore and friends have been deprecated for a
> year and building anything with __morecore ought to give the deprecation
> warning.

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.

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.

> I don't think we would like to export a different interface to do the same
> thing because it still doesn't allow us to clamp down further on the system
> malloc implementation.  Besides, the morecore hooking interface is severely
> limited because it only affects the main arena. Non-main arenas have their
> own allocation techniques and it doesn't really make sense to have an
> interface to control just the main arena.

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.

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 :-)

> There is only the one known issue that you reported but that may well be
> because the interface isn't well tested.  There have been many changes to
> malloc over the years and I cannot say for certain that the idea that
> morecore could be anything other than brk has been consistently considered.
> We don't have tests to verify that either. 

I guess you mean sbrk() above and not brk().

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 :-)

> Further, morecore limits its influence to the main arena and it is
> possible that future changes could break this in ways that we cannot
> anticipate today.
 
As I said, we're aware and it not ideal :-). But if you're using single
threaded software, that covers all allocations under the mmap threshold
(which is tunable). So it's _very_ easy to make this hook usable for a lot
of software. The limitations are not such a big deal (but again, I'd prefer
if they were not there).

> Basically this is technical debt we've accumulated from the days when malloc
> assumed single threaded programs and was incrementally hacked to add in
> multi-threaded support.  We'd like to clean this up.

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.
 
> The malloc code in glibc is quite general purpose and has little dependency
> on libc internals.  It may well be possible for you to copy the source files
> over into libhugetlbfs to implement the interposer within libhugetlbfs that
> does exactly what you need.  In fact, that may even allow you to extend
> hugetlbfs support to non-main arenas.

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.

Guillaume.
  

On 7/14/21 12:42 PM, Guillaume Morin wrote:
> 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.

Please work with upstream libhugetlbfs to prioritize issue #52.

Deprecation of morecore in glibc #52 (2020-08-27):
https://github.com/libhugetlbfs/libhugetlbfs/issues/52
  

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
  

On 14/07/2021 14:15, Carlos O'Donell via Libc-alpha wrote:
> On 7/14/21 12:42 PM, Guillaume Morin wrote:
>> 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.
> 
> Please work with upstream libhugetlbfs to prioritize issue #52.
> 
> Deprecation of morecore in glibc #52 (2020-08-27):
> https://github.com/libhugetlbfs/libhugetlbfs/issues/52
> 

I agree with Carlos and Siddhesh rationale that a dangling function
pointer in an essential code as malloc is a security liability and it 
is about time to get removed (as there is PoC out there that does it to 
exploit glibc malloc). However I think we can work towards a solution to
enable within glibc instead of pushing the support on libhugetlfs, which 
does *not* aim to be a malloc replacement but rather a way to explicit
provide and manage large pages.

There is some discussion last year about providing large pages support
directly on glibc without resorting to THP [1].  The idea is to hook
up the mmap/madvise with the required flags, which seems similar of
what is required by libhugetls morecore() implementation [2]. So I think
it should be feasible to add support for the required bits on glibc
and provide a tunable to actually use it. 

The proposed patchset does require some additional work, such as providing
is through a tunable, allowing different sizes depending of the architecture,
and maybe just dump the sbrk change in favor of just using mmap() for the 
case of large pages.

Guillaume, would this be suffice for your use cases.  The libhugetlsfs
projects does provide more functionality than glibc scope, for instance
a linker script to for bss/data and exe section to large parges, so I
glibc support would be a complement to what libhugetls provides.

[1] https://sourceware.org/pipermail/libc-alpha/2020-May/113539.html
[2] https://github.com/libhugetlbfs/libhugetlbfs/blob/master/morecore.c
  

On 14 Jul 23:02, Siddhesh Poyarekar wrote:
> 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.

As I said though, there is no other way besides forking glibc's malloc.
the morecore.c implementation in libhugetlbfs is like 100 lines of code.
We're not talking apples to apples. Far from it.

> 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 will certainly grant that someone should have reached out
sooner, though I would probably have done it if my bug report had not
been ignore for 5 years ;-)

> > 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.

I did not mean to say it should be moved to the debug DSO.

What I am saying is having explicit glibc support so I can LD_PRELOAD  a
version of libhugetlbfs that would export a __morecore() that will then be
used by glibc. No function pointers that needs explicitly set.

I am suggesting morecore interposition instead of the malloc
interposition you initially suggested. That should make no security
difference at all. It just requires to agree on some semantics though
for the interface which are basically these are sbrk.

> 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.

I think I have made decent technical points that it's not complicated at
all. There are no complications or security implications that I see,
just agreeing on the semantics on one function ...
I keep hearing theoretical concerns but nobody seems to want to detail:
why morecore interposition is complicated or insecure. I'll grant you
guys are the experts here, so please humor me, I am all ears.

> 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.

I have not argued for you to keep the hooks as is because i understand
your concerns. I am trying to find a solution that alleviates your
understandable concerns without breaking every single libhugetlbfs
user.

Guillaume.
  

On 14 Jul 13:15, Carlos O'Donell wrote:
>
> On 7/14/21 12:42 PM, Guillaume Morin wrote:
> > 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.
> 
> Please work with upstream libhugetlbfs to prioritize issue #52.
> 
> Deprecation of morecore in glibc #52 (2020-08-27):
> https://github.com/libhugetlbfs/libhugetlbfs/issues/52

I wrote quite a few extensive emails in this thread about how there are
no good, maintainbable solution for libhugetlbfs that I can see. If
there was, I would have written a patch already instead of arguing over
email :-) 

I do get your frustation here and libhugetlbfs users/debs should have
reached out earlier. But I cannot do much about that now. I am trying to
find a workable compromise here, that's all.
  

On 14 Jul 14:42, Adhemerval Zanella wrote:
> I agree with Carlos and Siddhesh rationale that a dangling function
> pointer in an essential code as malloc is a security liability and it 
> is about time to get removed (as there is PoC out there that does it to 
> exploit glibc malloc).

To reiterate, I have not argued to keep the hook as is. I do understand
the valid concerns about potential exploitation with the current scheme.

> However I think we can work towards a solution to enable within glibc
> instead of pushing the support on libhugetlfs, which does *not* aim to
> be a malloc replacement but rather a way to explicit provide and
> manage large pages.
> There is some discussion last year about providing large pages support
> directly on glibc without resorting to THP [1].  The idea is to hook
> up the mmap/madvise with the required flags, which seems similar of
> what is required by libhugetls morecore() implementation [2]. So I think
> it should be feasible to add support for the required bits on glibc
> and provide a tunable to actually use it. 

That would be ideal.

> Guillaume, would this be suffice for your use cases.  The libhugetlsfs
> projects does provide more functionality than glibc scope, for instance
> a linker script to for bss/data and exe section to large parges, so I
> glibc support would be a complement to what libhugetls provides.

Yes, it would be perfect for my use case.

Guillaume.
  

On 7/14/21 11:55 PM, Guillaume Morin wrote:
> I did not mean to say it should be moved to the debug DSO.
> 
> What I am saying is having explicit glibc support so I can LD_PRELOAD  a
> version of libhugetlbfs that would export a __morecore() that will then be
> used by glibc. No function pointers that needs explicitly set.
> 
> I am suggesting morecore interposition instead of the malloc
> interposition you initially suggested. That should make no security
> difference at all. It just requires to agree on some semantics though
> for the interface which are basically these are sbrk.

This seems to be the central the point of difference.  A contract for an 
interface like that is not limited to hugetlbfs.

That said, Adhemerval's suggestion is perhaps a good compromise.  I'm 
far more comfortable with a tunable chosen hugetlbfs morecore 
implementation within glibc since it does not have any arbitrary 
interface contracts that we may be obliged to maintain indefinitely.

Siddhesh
  

On 7/14/21 11:12 PM, Adhemerval Zanella wrote:
> There is some discussion last year about providing large pages support
> directly on glibc without resorting to THP [1].  The idea is to hook
> up the mmap/madvise with the required flags, which seems similar of
> what is required by libhugetls morecore() implementation [2]. So I think
> it should be feasible to add support for the required bits on glibc
> and provide a tunable to actually use it.

Sure, having hugetlb support within glibc is far more maintainable IMO 
than exposing a generic interface.

> The proposed patchset does require some additional work, such as providing
> is through a tunable, allowing different sizes depending of the architecture,
> and maybe just dump the sbrk change in favor of just using mmap() for the
> case of large pages.

I think we should keep it distinct from this patchset.  Since adding 
hugetlb support isn't likely to have an ABI impact (only tunables, which 
is not ABI), it could be done after 2.34.

What do you think?

Siddhesh
  

On 15 Jul  0:13, Siddhesh Poyarekar wrote:
> This seems to be the central the point of difference.  A contract for an
> interface like that is not limited to hugetlbfs.

I do agree that it does seem to the central point of contention. I am
still a little confused about why that is though. The contract is: pass
a size_t, return the previous break. No other inferface necessary, mo special case or
anything necessary in the malloc code. You only need to provide the
default implementation that is just a straightforward call to sbrk().

> That said, Adhemerval's suggestion is perhaps a good compromise.  I'm far
> more comfortable with a tunable chosen hugetlbfs morecore implementation
> within glibc since it does not have any arbitrary interface contracts that
> we may be obliged to maintain indefinitely.

If that's workable, I do prefer this solution as well. A tunable is
stricly better.
  

On 7/13/21 3:38 AM, Siddhesh Poyarekar wrote:
> Make the __morecore and __default_morecore symbols compat-only and
> remove their declarations from the API.  Also, include morecore.c
> directly into malloc.c; this should ideally get merged into malloc in
> a future cleanup.

OK for 2.34.

Tested without regression on x86_64 and i686.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  NEWS              |  5 +++++
>  include/stdlib.h  |  3 ---
>  malloc/Makefile   |  2 +-
>  malloc/arena.c    | 12 ++----------
>  malloc/hooks.c    |  2 ++
>  malloc/malloc.c   |  7 +++----
>  malloc/malloc.h   |  8 --------
>  malloc/morecore.c | 34 +++++++++++-----------------------
>  8 files changed, 24 insertions(+), 49 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 13ffe627da..18d9e65eb2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -113,6 +113,11 @@ Deprecated and removed features, and other changes affecting compatibility:
>    mtrace.  Similar functionality can be achieved by using conditional
>    breakpoints within mtrace functions from within gdb.
>  
> +* The __morecore and __after_morecore_hook malloc hooks and the default
> +  implementation __default_morecore have been removed from the API.  Existing
> +  applications will continue to link against these symbols but the interfaces
> +  no longer have any effect on malloc.

OK.

> +
>  Changes to build and runtime requirements:
>  
>  * On Linux, the shm_open, sem_open, and related functions now expect the
> diff --git a/include/stdlib.h b/include/stdlib.h
> index 1f6e1508e4..1c6f70b082 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -306,9 +306,6 @@ libc_hidden_proto (__qfcvt_r)
>  #  define MB_CUR_MAX (_NL_CURRENT_WORD (LC_CTYPE, _NL_CTYPE_MB_CUR_MAX))
>  # endif
>  
> -extern void *__default_morecore (ptrdiff_t) __THROW;
> -libc_hidden_proto (__default_morecore)

OK.

> -
>  struct abort_msg_s
>  {
>    unsigned int size;
> diff --git a/malloc/Makefile b/malloc/Makefile
> index d15729569b..020f781b59 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -104,7 +104,7 @@ tests-exclude-mcheck = tst-mallocstate \
>  tests-mcheck = $(filter-out $(tests-exclude-mcheck), $(tests))
>  endif
>  
> -routines = malloc morecore mcheck mtrace obstack reallocarray \
> +routines = malloc mcheck mtrace obstack reallocarray \

OK.

>    scratch_buffer_dupfree \
>    scratch_buffer_grow scratch_buffer_grow_preserve \
>    scratch_buffer_set_array_size \
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 991fc21a7e..f1693ed48f 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -274,14 +274,6 @@ next_env_entry (char ***position)
>  #endif
>  
>  
> -#if defined(SHARED) || defined(USE_MTAG)
> -static void *
> -__failing_morecore (ptrdiff_t d)
> -{
> -  return (void *) MORECORE_FAILURE;
> -}
> -#endif
> -

OK.

>  #ifdef SHARED
>  extern struct dl_open_hook *_dl_open_hook;
>  libc_hidden_proto (_dl_open_hook);
> @@ -310,7 +302,7 @@ ptmalloc_init (void)
>  	 and that morecore does not support tagged regions, then
>  	 disable it.  */
>        if (__MTAG_SBRK_UNTAGGED)
> -	__morecore = __failing_morecore;
> +	__always_fail_morecore = true;

OK.

>  
>        mtag_enabled = true;
>        mtag_mmap_flags = __MTAG_MMAP_FLAGS;
> @@ -323,7 +315,7 @@ ptmalloc_init (void)
>       generic sbrk implementation also enforces this, but it is not
>       used on Hurd.  */
>    if (!__libc_initial)
> -    __morecore = __failing_morecore;
> +    __always_fail_morecore = true;

OK.

>  #endif
>  
>    thread_arena = &main_arena;
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 45c91d6502..4aa6dadcff 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -20,6 +20,8 @@
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
>  void weak_variable (*__after_morecore_hook) (void) = NULL;
>  compat_symbol (libc, __after_morecore_hook, __after_morecore_hook, GLIBC_2_0);
> +void *(*__morecore)(ptrdiff_t);
> +compat_symbol (libc, __morecore, __morecore, GLIBC_2_0);

OK.

>  #endif
>  
>  /* Hooks for debugging versions.  The initial hooks just call the
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 24e7854a0e..6e8fa9e424 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -384,12 +384,11 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
>  #define TRIM_FASTBINS  0
>  #endif
>  
> -
>  /* Definition for getting more memory from the OS.  */
> -#define MORECORE         (*__morecore)
> +#include "morecore.c"
> +
> +#define MORECORE         (*__glibc_morecore)

OK.

>  #define MORECORE_FAILURE 0
> -void * __default_morecore (ptrdiff_t);
> -void *(*__morecore)(ptrdiff_t) = __default_morecore;

OK.

>  
>  /* Memory tagging.  */
>  
> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 634b7db868..17ab9ee345 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -76,14 +76,6 @@ extern void *valloc (size_t __size) __THROW __attribute_malloc__
>  extern void *pvalloc (size_t __size) __THROW __attribute_malloc__
>    __wur __attr_dealloc_free;
>  
> -/* Underlying allocation function; successive calls should return
> -   contiguous pieces of memory.  */
> -extern void *(*__morecore) (ptrdiff_t __size) __MALLOC_DEPRECATED;
> -
> -/* Default value of `__morecore'.  */
> -extern void *__default_morecore (ptrdiff_t __size)
> -__THROW __attribute_malloc__  __MALLOC_DEPRECATED;

OK.

> -
>  /* SVID2/XPG mallinfo structure */
>  
>  struct mallinfo
> diff --git a/malloc/morecore.c b/malloc/morecore.c
> index 047228779b..8168ef158c 100644
> --- a/malloc/morecore.c
> +++ b/malloc/morecore.c
> @@ -15,39 +15,27 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#ifndef _MALLOC_INTERNAL
> -# define _MALLOC_INTERNAL
> -# include <malloc.h>
> -#endif
> -
> -#ifndef __GNU_LIBRARY__
> -# define __sbrk  sbrk
> -#endif
> -
> -#ifdef __GNU_LIBRARY__
> -/* It is best not to declare this and cast its result on foreign operating
> -   systems with potentially hostile include files.  */
> -
> -# include <stddef.h>
> -# include <stdlib.h>
> -extern void *__sbrk (ptrdiff_t increment) __THROW;
> -libc_hidden_proto (__sbrk)
> -#endif
> -
> -#ifndef NULL
> -# define NULL 0

OK.

> +#if defined(SHARED) || defined(USE_MTAG)
> +static bool __always_fail_morecore = false;
>  #endif
>  
>  /* Allocate INCREMENT more bytes of data space,
>     and return the start of data space, or NULL on errors.
>     If INCREMENT is negative, shrink data space.  */
>  void *
> -__default_morecore (ptrdiff_t increment)
> +__glibc_morecore (ptrdiff_t increment)
>  {
> +#if defined(SHARED) || defined(USE_MTAG)
> +  if (__always_fail_morecore)
> +    return NULL;
> +#endif

OK.

> +
>    void *result = (void *) __sbrk (increment);
>    if (result == (void *) -1)
>      return NULL;
>  
>    return result;
>  }
> -libc_hidden_def (__default_morecore)
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_34)
> +compat_symbol (libc, __glibc_morecore, __default_morecore, GLIBC_2_0);

OK. Use a new name and create a versioned compat symbol for __default_morecore
in case someone calls it.

> +#endif
>