[v4,00/10] Remove malloc hooks

  This patchset removes the malloc hooks __malloc_hook, __free_hook,
__realloc_hook and __memalign_hook from the API and leaves compatibility
symbols so that existing applications can continue to link to them.  The
reading and execution of the hooks has been moved to a DSO
libmalloc_compathooks.so, which can be preloaded for applications that
need it.  By default these hooks no longer have any effect in the
library.

Further, the __morecore, __morecore_after_hook and __default_morecore
hooks have also been moved to compat symbols and removed from the API.
Existing applications will continue to link to them but they won't have
any effect on malloc behaviour.

To enable this, the MALLOC_CHECK_, mcheck() and mtrace() hooks have been
weaned away from hooks.  In the process, some mcheck() failures have
been fixed and the overall behaviour is now simpler to follow.  Finally,
tr_break and mallwatch symbols have been deprecated and are not used
anywhere.  Users are advised to use gdb watchpoints and conditional
breakpoints to debug malloc internals since they ought to provide
equivalent functionality.

Changes from v3:
- Remove source file dependencies
- Commit mcheck tests

Changes from v2:
- Move hooks dependencies to malloc.o{,sS}

Changes from v1:

- Added makefile dependencies for the new hooks files
- Fixed memset call in calloc debugging hooks
- Added the tr_break deprecation patch and mcheck test patch to this
  series

Siddhesh Poyarekar (10):
  Drop source dependencies on hooks.c and arena.c
  mtrace: Deprecate mallwatch and tr_break
  Move glibc.malloc.check implementation into its own file
  malloc: Move malloc hook references to hooks.c
  glibc.malloc.check: Wean away from malloc hooks
  mcheck: Wean away from malloc hooks
  mtrace: Wean away from malloc hooks
  Remove malloc hooks
  Remove __after_morecore_hook
  Remove __morecore and __default_morecore

 Makeconfig                                    |   2 +-
 NEWS                                          |  18 +
 include/malloc.h                              |  11 +-
 include/stdlib.h                              |   3 -
 malloc/Makefile                               |  23 +-
 malloc/Versions                               |   3 +
 malloc/arena.c                                |  26 +-
 malloc/hooks.c                                | 545 +++++++-----------
 malloc/malloc-check.c                         | 376 ++++++++++++
 malloc/malloc-compathooks.c                   | 166 ++++++
 malloc/malloc-internal.h                      |   6 +
 malloc/malloc.c                               | 235 ++++----
 malloc/malloc.h                               |  27 -
 malloc/mcheck-hooks.c                         | 411 +++++++++++++
 malloc/mcheck-init.c                          |  14 +-
 malloc/mcheck.c                               | 369 +-----------
 malloc/morecore.c                             |  15 +-
 malloc/mtrace-hooks.c                         | 137 +++++
 malloc/mtrace.c                               | 276 +--------
 manual/memory.texi                            | 191 +-----
 sysdeps/mach/hurd/i386/libc.abilist           |   1 +
 sysdeps/unix/sysv/linux/aarch64/libc.abilist  |   1 +
 sysdeps/unix/sysv/linux/alpha/libc.abilist    |   1 +
 sysdeps/unix/sysv/linux/arc/libc.abilist      |   1 +
 sysdeps/unix/sysv/linux/arm/be/libc.abilist   |   1 +
 sysdeps/unix/sysv/linux/arm/le/libc.abilist   |   1 +
 sysdeps/unix/sysv/linux/csky/libc.abilist     |   1 +
 sysdeps/unix/sysv/linux/hppa/libc.abilist     |   1 +
 sysdeps/unix/sysv/linux/i386/libc.abilist     |   1 +
 sysdeps/unix/sysv/linux/ia64/libc.abilist     |   1 +
 .../sysv/linux/m68k/coldfire/libc.abilist     |   1 +
 .../unix/sysv/linux/m68k/m680x0/libc.abilist  |   1 +
 .../sysv/linux/microblaze/be/libc.abilist     |   1 +
 .../sysv/linux/microblaze/le/libc.abilist     |   1 +
 .../sysv/linux/mips/mips32/fpu/libc.abilist   |   1 +
 .../sysv/linux/mips/mips32/nofpu/libc.abilist |   1 +
 .../sysv/linux/mips/mips64/n32/libc.abilist   |   1 +
 .../sysv/linux/mips/mips64/n64/libc.abilist   |   1 +
 sysdeps/unix/sysv/linux/nios2/libc.abilist    |   1 +
 .../linux/powerpc/powerpc32/fpu/libc.abilist  |   1 +
 .../powerpc/powerpc32/nofpu/libc.abilist      |   1 +
 .../linux/powerpc/powerpc64/be/libc.abilist   |   1 +
 .../linux/powerpc/powerpc64/le/libc.abilist   |   1 +
 .../unix/sysv/linux/riscv/rv32/libc.abilist   |   1 +
 .../unix/sysv/linux/riscv/rv64/libc.abilist   |   1 +
 .../unix/sysv/linux/s390/s390-32/libc.abilist |   1 +
 .../unix/sysv/linux/s390/s390-64/libc.abilist |   1 +
 sysdeps/unix/sysv/linux/sh/be/libc.abilist    |   1 +
 sysdeps/unix/sysv/linux/sh/le/libc.abilist    |   1 +
 .../sysv/linux/sparc/sparc32/libc.abilist     |   1 +
 .../sysv/linux/sparc/sparc64/libc.abilist     |   1 +
 .../unix/sysv/linux/x86_64/64/libc.abilist    |   1 +
 .../unix/sysv/linux/x86_64/x32/libc.abilist   |   1 +
 53 files changed, 1547 insertions(+), 1340 deletions(-)
 create mode 100644 malloc/malloc-check.c
 create mode 100644 malloc/malloc-compathooks.c
 create mode 100644 malloc/mcheck-hooks.c
 create mode 100644 malloc/mtrace-hooks.c
  

On 7/2/21 7:38 AM, Siddhesh Poyarekar wrote:
> This patchset removes the malloc hooks __malloc_hook, __free_hook,
> __realloc_hook and __memalign_hook from the API and leaves compatibility
> symbols so that existing applications can continue to link to them.  The
> reading and execution of the hooks has been moved to a DSO
> libmalloc_compathooks.so, which can be preloaded for applications that
> need it.  By default these hooks no longer have any effect in the
> library.

OK. Good.
 
> Further, the __morecore, __morecore_after_hook and __default_morecore
> hooks have also been moved to compat symbols and removed from the API.
> Existing applications will continue to link to them but they won't have
> any effect on malloc behaviour.

OK. Good.

> To enable this, the MALLOC_CHECK_, mcheck() and mtrace() hooks have been
> weaned away from hooks.  In the process, some mcheck() failures have
> been fixed and the overall behaviour is now simpler to follow.  Finally,
> tr_break and mallwatch symbols have been deprecated and are not used
> anywhere.  Users are advised to use gdb watchpoints and conditional
> breakpoints to debug malloc internals since they ought to provide
> equivalent functionality.

OK.

Thanks for working through this.

This is looking better but we need to discuss the mcheck status and
dropping some of those interfaces, perhaps just deprecating libmcheck.a
and moving the functionality into libc_malloc_debug.so. See further review.

I think we're almost done with a v4 review if we agree on direction.

> Changes from v3:
> - Remove source file dependencies
> - Commit mcheck tests
> 
> Changes from v2:
> - Move hooks dependencies to malloc.o{,sS}
> 
> Changes from v1:
> 
> - Added makefile dependencies for the new hooks files
> - Fixed memset call in calloc debugging hooks
> - Added the tr_break deprecation patch and mcheck test patch to this
>   series
> 
> Siddhesh Poyarekar (10):
>   Drop source dependencies on hooks.c and arena.c
>   mtrace: Deprecate mallwatch and tr_break
>   Move glibc.malloc.check implementation into its own file
>   malloc: Move malloc hook references to hooks.c
>   glibc.malloc.check: Wean away from malloc hooks
>   mcheck: Wean away from malloc hooks
>   mtrace: Wean away from malloc hooks
>   Remove malloc hooks
>   Remove __after_morecore_hook
>   Remove __morecore and __default_morecore
> 
>  Makeconfig                                    |   2 +-
>  NEWS                                          |  18 +
>  include/malloc.h                              |  11 +-
>  include/stdlib.h                              |   3 -
>  malloc/Makefile                               |  23 +-
>  malloc/Versions                               |   3 +
>  malloc/arena.c                                |  26 +-
>  malloc/hooks.c                                | 545 +++++++-----------
>  malloc/malloc-check.c                         | 376 ++++++++++++
>  malloc/malloc-compathooks.c                   | 166 ++++++
>  malloc/malloc-internal.h                      |   6 +
>  malloc/malloc.c                               | 235 ++++----
>  malloc/malloc.h                               |  27 -
>  malloc/mcheck-hooks.c                         | 411 +++++++++++++
>  malloc/mcheck-init.c                          |  14 +-
>  malloc/mcheck.c                               | 369 +-----------
>  malloc/morecore.c                             |  15 +-
>  malloc/mtrace-hooks.c                         | 137 +++++
>  malloc/mtrace.c                               | 276 +--------
>  manual/memory.texi                            | 191 +-----
>  sysdeps/mach/hurd/i386/libc.abilist           |   1 +
>  sysdeps/unix/sysv/linux/aarch64/libc.abilist  |   1 +
>  sysdeps/unix/sysv/linux/alpha/libc.abilist    |   1 +
>  sysdeps/unix/sysv/linux/arc/libc.abilist      |   1 +
>  sysdeps/unix/sysv/linux/arm/be/libc.abilist   |   1 +
>  sysdeps/unix/sysv/linux/arm/le/libc.abilist   |   1 +
>  sysdeps/unix/sysv/linux/csky/libc.abilist     |   1 +
>  sysdeps/unix/sysv/linux/hppa/libc.abilist     |   1 +
>  sysdeps/unix/sysv/linux/i386/libc.abilist     |   1 +
>  sysdeps/unix/sysv/linux/ia64/libc.abilist     |   1 +
>  .../sysv/linux/m68k/coldfire/libc.abilist     |   1 +
>  .../unix/sysv/linux/m68k/m680x0/libc.abilist  |   1 +
>  .../sysv/linux/microblaze/be/libc.abilist     |   1 +
>  .../sysv/linux/microblaze/le/libc.abilist     |   1 +
>  .../sysv/linux/mips/mips32/fpu/libc.abilist   |   1 +
>  .../sysv/linux/mips/mips32/nofpu/libc.abilist |   1 +
>  .../sysv/linux/mips/mips64/n32/libc.abilist   |   1 +
>  .../sysv/linux/mips/mips64/n64/libc.abilist   |   1 +
>  sysdeps/unix/sysv/linux/nios2/libc.abilist    |   1 +
>  .../linux/powerpc/powerpc32/fpu/libc.abilist  |   1 +
>  .../powerpc/powerpc32/nofpu/libc.abilist      |   1 +
>  .../linux/powerpc/powerpc64/be/libc.abilist   |   1 +
>  .../linux/powerpc/powerpc64/le/libc.abilist   |   1 +
>  .../unix/sysv/linux/riscv/rv32/libc.abilist   |   1 +
>  .../unix/sysv/linux/riscv/rv64/libc.abilist   |   1 +
>  .../unix/sysv/linux/s390/s390-32/libc.abilist |   1 +
>  .../unix/sysv/linux/s390/s390-64/libc.abilist |   1 +
>  sysdeps/unix/sysv/linux/sh/be/libc.abilist    |   1 +
>  sysdeps/unix/sysv/linux/sh/le/libc.abilist    |   1 +
>  .../sysv/linux/sparc/sparc32/libc.abilist     |   1 +
>  .../sysv/linux/sparc/sparc64/libc.abilist     |   1 +
>  .../unix/sysv/linux/x86_64/64/libc.abilist    |   1 +
>  .../unix/sysv/linux/x86_64/x32/libc.abilist   |   1 +
>  53 files changed, 1547 insertions(+), 1340 deletions(-)
>  create mode 100644 malloc/malloc-check.c
>  create mode 100644 malloc/malloc-compathooks.c
>  create mode 100644 malloc/mcheck-hooks.c
>  create mode 100644 malloc/mtrace-hooks.c
>
  

On 7/3/21 12:35 AM, Carlos O'Donell wrote:
> Thanks for working through this.
> 
> This is looking better but we need to discuss the mcheck status and
> dropping some of those interfaces, perhaps just deprecating libmcheck.a
> and moving the functionality into libc_malloc_debug.so. See further review.
> 
> I think we're almost done with a v4 review if we agree on direction.

Thanks, I'm kinda emotional about malloc-check since it's bailed me out 
a number of times when valgrind/asan couldn't, but I suppose moving it 
along with the other debugging features to a different DSO isn't that 
bad.  I'll keep them mostly weaned off the hooks but also move them to 
the DSO.  Since it's in the DSO, mcheck can continue using the 
__malloc_initialize_hook to run the first mcheck() and we don't need a 
new ABI.

I'll commit the first 4 patches (since they're source shuffling anyway) 
and make a v5 from the rest.  Can I take till about Monday night to post 
the next set?  I think it would be a pretty big security gain for us to 
finally move the debugging hooks out of the main library for 2.34.

Thanks,
Siddhesh
  

On 7/3/21 12:45 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> I'll commit the first 4 patches (since they're source shuffling anyway) 

I only pushed the first 3.  I'll need the 4th one to make the next 
patchset cleaner now that we don't want any trace of debugging in the 
main malloc.

Siddhesh