This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] malloc: Remove malloc_get_state, malloc_set_state
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Fri, 10 Jun 2016 12:07:15 -0400
- Subject: Re: [PATCH] malloc: Remove malloc_get_state, malloc_set_state
- Authentication-results: sourceware.org; auth=none
- References: <20160610093137 dot 7306E400F0E5F at oldenburg dot str dot redhat dot com>
On 06/10/2016 05:31 AM, Florian Weimer wrote:
> After the removal of __malloc_initialize_hook, newly compiled
> Emacs binaries are no longer able to use these interfaces.
> malloc_get_state is only used during the Emacs build process,
> so we provide a stub implementation only. Existing Emacs binaries
> will not call this stub function, but still reference the symbol.
TLDR; Update news, and wiki, and this looks good to me.
This is a single cycle deprecation for a function which we have been saying
is going to be removed for the last 5 years. Given that we have shown that
nothing in distribution but Emacs uses this, that's fine with me.
However, I would like the release notes updated to mention that there
are going to be packaging changes required or user application changes
required if they were using these APIs.
e.g.
https://sourceware.org/glibc/wiki/Release/2.24#Packaging_Changes
> 2016-06-10 Florian Weimer <fweimer@redhat.com>
>
> * malloc/malloc.h (malloc_get_state, malloc_set_state): Remove.
> * malloc/malloc.c (malloc_get_state, malloc_set_state): Remove
> weak aliases.
> * malloc/hooks.c (malloc_get_state): New stub implementation as
> compatibility symbo.
s/symbo/symbol/g.
> (__malloc_get_state): Remove.
> (malloc_set_state): Rename from __malloc_set_state. Turn into
> compat symbol.
> * malloc/Makefile (tests): Remove tst-mallocstate.
> * malloc/tst-mallocstate.c: Remove file.
Minor comment about the NEWS entry.
> diff --git a/NEWS b/NEWS
> index 9d6ac56..de2af85 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -36,6 +36,13 @@ Version 2.24
> * The deprecated __malloc_initialize_hook variable has been removed from the
> API.
>
> +* The __malloc_get_state and __malloc_set_state functions have been removed
> + from the API. __malloc_get_state has been replaced with a stub
> + implementation. Existing undumped Emacs binaries will have to be
> + recompiled so that they do not use glibc malloc (or malloc heap dumping).
> + Existing installed Emacs binaries (after dumping) are not affected by this
> + change.
This needs to explain that an "Emacs" you use in a distribution is an installed
dumped Emacs.
e.g. Add "Emacs users should not see any impact on their installed distribution
binaries of Emacs."
Or something like that.
> +
> Security related changes:
>
> * An unnecessary stack copy in _nss_dns_getnetbyname_r was removed. It
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 91eb17d..8473f74 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -25,7 +25,7 @@ include ../Makeconfig
> dist-headers := malloc.h
> headers := $(dist-headers) obstack.h mcheck.h
> tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
> - tst-mallocstate tst-mcheck tst-mallocfork tst-trim1 \
> + tst-mcheck tst-mallocfork tst-trim1 \
> tst-malloc-usable tst-realloc tst-posix_memalign \
> tst-pvalloc tst-memalign tst-mallopt tst-scratch_buffer \
> tst-malloc-backtrace tst-malloc-thread-exit \
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index caa1e70..8ef2eba 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -447,6 +447,7 @@ memalign_check (size_t alignment, size_t bytes, const void *caller)
> return mem2mem_check (mem, bytes);
> }
>
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_24)
>
> /* Get/set state: malloc_get_state() records the current state of all
> malloc variables (_except_ for the actual heap contents and `hook'
> @@ -492,60 +493,20 @@ struct malloc_save_state
> unsigned long narenas;
> };
>
> +/* Dummy implementation which always fails. We need to provide this
> + symbol so that existing Emacs binaries continue to work with
> + BIND_NOW. */
> void *
> -__malloc_get_state (void)
> +attribute_compat_text_section
> +malloc_get_state (void)
> {
> - struct malloc_save_state *ms;
> - int i;
> - mbinptr b;
> -
> - ms = (struct malloc_save_state *) __libc_malloc (sizeof (*ms));
> - if (!ms)
> - return 0;
> -
> - (void) mutex_lock (&main_arena.mutex);
> - malloc_consolidate (&main_arena);
> - ms->magic = MALLOC_STATE_MAGIC;
> - ms->version = MALLOC_STATE_VERSION;
> - ms->av[0] = 0;
> - ms->av[1] = 0; /* used to be binblocks, now no longer used */
> - ms->av[2] = top (&main_arena);
> - ms->av[3] = 0; /* used to be undefined */
> - for (i = 1; i < NBINS; i++)
> - {
> - b = bin_at (&main_arena, i);
> - if (first (b) == b)
> - ms->av[2 * i + 2] = ms->av[2 * i + 3] = 0; /* empty bin */
> - else
> - {
> - ms->av[2 * i + 2] = first (b);
> - ms->av[2 * i + 3] = last (b);
> - }
> - }
> - ms->sbrk_base = mp_.sbrk_base;
> - ms->sbrked_mem_bytes = main_arena.system_mem;
> - ms->trim_threshold = mp_.trim_threshold;
> - ms->top_pad = mp_.top_pad;
> - ms->n_mmaps_max = mp_.n_mmaps_max;
> - ms->mmap_threshold = mp_.mmap_threshold;
> - ms->check_action = check_action;
> - ms->max_sbrked_mem = main_arena.max_system_mem;
> - ms->max_total_mem = 0;
> - ms->n_mmaps = mp_.n_mmaps;
> - ms->max_n_mmaps = mp_.max_n_mmaps;
> - ms->mmapped_mem = mp_.mmapped_mem;
> - ms->max_mmapped_mem = mp_.max_mmapped_mem;
> - ms->using_malloc_checking = using_malloc_checking;
> - ms->max_fast = get_max_fast ();
> - ms->arena_test = mp_.arena_test;
> - ms->arena_max = mp_.arena_max;
> - ms->narenas = narenas;
> - (void) mutex_unlock (&main_arena.mutex);
> - return (void *) ms;
> + return NULL;
> }
> +compat_symbol (libc, malloc_get_state, malloc_get_state, GLIBC_2_0);
>
> int
> -__malloc_set_state (void *msptr)
> +attribute_compat_text_section
> +malloc_set_state (void *msptr)
> {
> struct malloc_save_state *ms = (struct malloc_save_state *) msptr;
>
> @@ -612,6 +573,9 @@ __malloc_set_state (void *msptr)
>
> return 0;
> }
> +compat_symbol (libc, malloc_set_state, malloc_set_state, GLIBC_2_0);
> +
> +#endif /* SHLIB_COMPAT */
>
> /*
> * Local variables:
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index ac0f751..b054fe6 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -5266,8 +5266,6 @@ strong_alias (__libc_mallopt, __mallopt) weak_alias (__libc_mallopt, mallopt)
> weak_alias (__malloc_stats, malloc_stats)
> weak_alias (__malloc_usable_size, malloc_usable_size)
> weak_alias (__malloc_trim, malloc_trim)
> -weak_alias (__malloc_get_state, malloc_get_state)
> -weak_alias (__malloc_set_state, malloc_set_state)
>
>
> /* ------------------------------------------------------------
> diff --git a/malloc/malloc.h b/malloc/malloc.h
> index 54b1862..e0c2788 100644
> --- a/malloc/malloc.h
> +++ b/malloc/malloc.h
> @@ -134,13 +134,6 @@ extern void malloc_stats (void) __THROW;
> /* Output information about state of allocator to stream FP. */
> extern int malloc_info (int __options, FILE *__fp) __THROW;
>
> -/* Record the state of all malloc variables in an opaque data structure. */
> -extern void *malloc_get_state (void) __THROW;
> -
> -/* Restore the state of all malloc variables from data obtained with
> - malloc_get_state(). */
> -extern int malloc_set_state (void *__ptr) __THROW;
> -
> /* Hooks for debugging and user-defined versions. */
> extern void (*__MALLOC_HOOK_VOLATILE __free_hook) (void *__ptr,
> const void *)
> diff --git a/malloc/tst-mallocstate.c b/malloc/tst-mallocstate.c
> deleted file mode 100644
> index a00d045..0000000
> --- a/malloc/tst-mallocstate.c
> +++ /dev/null
> @@ -1,84 +0,0 @@
> -/* Copyright (C) 2001-2016 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> - Contributed by Wolfram Gloger <wg@malloc.de>, 2001.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <http://www.gnu.org/licenses/>. */
> -
> -#include <errno.h>
> -#include <stdio.h>
> -#include "malloc.h"
> -
> -static int errors = 0;
> -
> -static void
> -merror (const char *msg)
> -{
> - ++errors;
> - printf ("Error: %s\n", msg);
> -}
> -
> -static int
> -do_test (void)
> -{
> - void *p1, *p2;
> - void *save_state;
> - long i;
> -
> - errno = 0;
> -
> - p1 = malloc (10);
> - if (p1 == NULL)
> - merror ("malloc (10) failed.");
> -
> - p2 = malloc (20);
> - if (p2 == NULL)
> - merror ("malloc (20) failed.");
> -
> - free (malloc (10));
> -
> - for (i = 0; i < 100; ++i)
> - {
> - save_state = malloc_get_state ();
> - if (save_state == NULL)
> - {
> - merror ("malloc_get_state () failed.");
> - break;
> - }
> - /*free (malloc (10)); This could change the top chunk! */
> - malloc_set_state (save_state);
> - p1 = realloc (p1, i * 4 + 4);
> - if (p1 == NULL)
> - merror ("realloc (i*4) failed.");
> - free (save_state);
> - }
> -
> - p1 = realloc (p1, 40);
> - free (p2);
> - p2 = malloc (10);
> - if (p2 == NULL)
> - merror ("malloc (10) failed.");
> - free (p1);
> -
> - return errors != 0;
> -}
> -
> -/*
> - * Local variables:
> - * c-basic-offset: 2
> - * End:
> - */
> -
> -#define TEST_FUNCTION do_test ()
> -#include "../test-skeleton.c"
>
--
Cheers,
Carlos.