Re: [PATCH] free: preserve errno [BZ#17924]

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 12/21/20 1:55 AM, Paul Eggert wrote:
> * malloc/Makefile (tests): Add tst-free-errno.
> * malloc/malloc.c (tcache_init): Preserve errno when initializing,
> since 'free' might be calling us.
> (__libc_free): Preserve errno when calling munmap.
> * malloc/tst-free-errno.c: New file, almost all from Bruno Haible.
> * manual/memory.texi (Freeing after Malloc, Replacing malloc):
> Document that free preserves errno.
> ---
>   malloc/Makefile         |   1 +
>   malloc/malloc.c         |   6 ++
>   malloc/tst-free-errno.c | 169 ++++++++++++++++++++++++++++++++++++++++
>   manual/memory.texi      |   9 +++
>   4 files changed, 185 insertions(+)
>   create mode 100644 malloc/tst-free-errno.c
> 
> diff --git a/malloc/Makefile b/malloc/Makefile
> index ab64dcfd73..4b3975f90d 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -34,6 +34,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>   	 tst-interpose-nothread \
>   	 tst-interpose-thread \
>   	 tst-alloc_buffer \
> +	 tst-free-errno \
>   	 tst-malloc-tcache-leak \
>   	 tst-malloc_info tst-mallinfo2 \
>   	 tst-malloc-too-large \
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 326075e704..14bc55f96d 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3003,6 +3003,8 @@ tcache_init(void)
>     if (tcache_shutting_down)
>       return;
>   
> +  int err = errno;
> +
>     arena_get (ar_ptr, bytes);
>     victim = _int_malloc (ar_ptr, bytes);
>     if (!victim && ar_ptr != NULL)
> @@ -3015,6 +3017,8 @@ tcache_init(void)
>     if (ar_ptr != NULL)
>       __libc_lock_unlock (ar_ptr->mutex);
>   
> +  __set_errno (err);
> +
>     /* In a low memory situation, we may not be able to allocate memory
>        - in which case, we just keep trying later.  However, we
>        typically do this very early, so either there is sufficient
> @@ -3140,7 +3144,9 @@ __libc_free (void *mem)
>             LIBC_PROBE (memory_mallopt_free_dyn_thresholds, 2,
>                         mp_.mmap_threshold, mp_.trim_threshold);
>           }
> +      int err = errno;
>         munmap_chunk (p);
> +      __set_errno (err);
>         return;
>       }

The _int_free call after this may trigger a trim, which could result in 
an mmap, madvise or brk, all of which set errno.  In practice I think 
only mmap may do this but it's reason enough to perhaps just save errno 
at the top and restore it on exit.

The mmap is called when /proc/sys/vm/overcommit_memory has 2.

>   
> diff --git a/malloc/tst-free-errno.c b/malloc/tst-free-errno.c
> new file mode 100644
> index 0000000000..6243cb6e0b
> --- /dev/null
> +++ b/malloc/tst-free-errno.c
> @@ -0,0 +1,169 @@
> +/* Test that free preserves errno.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* Written by Bruno Haible <bruno@clisp.org>, 2020.  */

We don't write these anymore.

> +
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#if defined __linux__
> +# include <fcntl.h>
> +# include <stdint.h>
> +# include <string.h>
> +# include <sys/mman.h>
> +#endif
> +
> +#define ASSERT_NO_STDIO(expr) \
> +  do                                                        \
> +    {                                                       \
> +      if (!(expr))                                          \
> +        {                                                   \
> +          WRITE_TO_STDERR (__FILE__);                       \
> +          WRITE_TO_STDERR (":");                            \
> +          WRITE_MACROEXPANDED_INTEGER_TO_STDERR (__LINE__); \
> +          WRITE_TO_STDERR (": assertion '");                \
> +          WRITE_TO_STDERR (#expr);                          \
> +          WRITE_TO_STDERR ("' failed\n");                   \
> +          abort ();                                         \
> +        }                                                   \
> +    }                                                       \
> +  while (0)
> +#define WRITE_MACROEXPANDED_INTEGER_TO_STDERR(integer) \
> +  WRITE_INTEGER_TO_STDERR(integer)
> +#define WRITE_INTEGER_TO_STDERR(integer) \
> +  WRITE_TO_STDERR (#integer)
> +#define WRITE_TO_STDERR(string_literal) \
> +  {                                     \
> +    const char *s = string_literal;     \
> +    int ret = write (2, s, strlen (s)); \
> +    (void) ret;                         \
> +  }
> +
> +/* The indirection through a volatile function pointer is necessary to prevent
> +   a GCC optimization.  Without it, when optimizing, GCC would "know" that errno
> +   is unchanged by calling free(ptr), when ptr was the result of a malloc(...)
> +   call in the same function.  */
> +static int
> +get_errno (void)
> +{
> +  volatile int err = errno;
> +  return err;
> +}
> +
> +static int (* volatile get_errno_func) (void) = get_errno;
> +
> +static int
> +do_test (void)
> +{
> +  /* Check that free() preserves errno.  */
> +  {
> +    errno = 1789; /* Liberté, égalité, fraternité.  */
> +    free (NULL);
> +    ASSERT_NO_STDIO (get_errno_func () == 1789);
> +  }
> +  { /* Large memory allocations.  */
> +    #define N 2
> +    void * volatile ptrs[N];
> +    size_t i;
> +    for (i = 0; i < N; i++)
> +      ptrs[i] = malloc (5318153);
> +    for (i = 0; i < N; i++)
> +      {
> +        errno = 1789;
> +        free (ptrs[i]);
> +        ASSERT_NO_STDIO (get_errno_func () == 1789);
> +      }
> +    #undef N
> +  }
> +
> +  /* Test a less common code path.
> +     When malloc() is based on mmap(), free() can sometimes call munmap().
> +     munmap() usually succeeds, but fails in a particular situation: when
> +       - it has to unmap the middle part of a VMA, and
> +       - the number of VMAs of a process is limited and the limit is
> +         already reached.
> +     The latter condition is fulfilled on Linux, when the file
> +     /proc/sys/vm/max_map_count exists.  This file contains the limit
> +       - for Linux >= 2.4.19: 65536 (DEFAULT_MAX_MAP_COUNT in linux/include/linux/sched.h)
> +       - for Linux >= 2.6.31: 65530 (DEFAULT_MAX_MAP_COUNT in linux/include/linux/mm.h).
> +   */
> +  #if defined __linux__
> +  if (open ("/proc/sys/vm/max_map_count", O_RDONLY) >= 0)
> +    {
> +      /* Preparations.  */
> +      size_t pagesize = getpagesize ();
> +      void *firstpage_backup = malloc (pagesize);
> +      void *lastpage_backup = malloc (pagesize);
> +      /* Allocate a large memory area, as a bumper, so that the MAP_FIXED
> +         allocation later will not overwrite parts of the memory areas
> +         allocated to ld.so or libc.so.  */
> +      void *bumper_region =
> +        mmap (NULL, 0x1000000, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +      /* A file descriptor pointing to a regular file.  */
> +      int fd = open ("/etc/hosts", O_RDONLY);
> +
> +      if (firstpage_backup != NULL && lastpage_backup != NULL
> +          && bumper_region != (void *)(-1)
> +          && fd >= 0)
> +        {
> +          /* Do a large memory allocation.  */
> +          size_t big_size = 0x1000000;
> +          void * volatile ptr = malloc (big_size - 0x100);
> +          char *ptr_aligned = (char *) ((uintptr_t) ptr & ~(pagesize - 1));
> +          /* This large memory allocation allocated a memory area
> +             from ptr_aligned to ptr_aligned + big_size.
> +             Enlarge this memory area by adding a page before and a page
> +             after it.  */
> +          memcpy (firstpage_backup, ptr_aligned, pagesize);
> +          memcpy (lastpage_backup, ptr_aligned + big_size - pagesize, pagesize);
> +          if (mmap (ptr_aligned - pagesize, pagesize + big_size + pagesize,
> +                    PROT_READ | PROT_WRITE,
> +                    MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0)
> +              != (void *)(-1))
> +            {
> +              memcpy (ptr_aligned, firstpage_backup, pagesize);
> +              memcpy (ptr_aligned + big_size - pagesize, lastpage_backup, pagesize);
> +
> +              /* Now add as many mappings as we can.
> +                 Stop at 65536, in order not to crash the machine (in case the
> +                 limit has been increased by the system administrator).  */
> +              size_t i;
> +              for (i = 0; i < 65536; i++)
> +                if (mmap (NULL, pagesize, PROT_READ, MAP_FILE | MAP_PRIVATE, fd, 0)
> +                    == (void *)(-1))
> +                  break;
> +              /* Now the number of VMAs of this process has hopefully attained
> +                 its limit.  */
> +
> +              errno = 1789;
> +              /* This call to free() is supposed to call
> +                   munmap (ptr_aligned, big_size);
> +                 which increases the number of VMAs by 1, which is supposed
> +                 to fail.  */
> +              free (ptr);
> +              ASSERT_NO_STDIO (get_errno_func () == 1789);
> +            }
> +        }
> +    }
> +  #endif
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/manual/memory.texi b/manual/memory.texi
> index c132261084..b2cc65228a 100644
> --- a/manual/memory.texi
> +++ b/manual/memory.texi
> @@ -738,6 +738,12 @@ later call to @code{malloc} to reuse the space.  In the meantime, the
>   space remains in your program as part of a free-list used internally by
>   @code{malloc}.
>   
> +The @code{free} function preserves the value of @code{errno}, so that
> +cleanup code need not worry about saving and restoring @code{errno}
> +around a call to @code{free}.  Although neither @w{ISO C} nor
> +POSIX.1-2017 requires @code{free} to preserve @code{errno}, a future
> +version of POSIX is planned to require it.
> +
>   There is no point in freeing blocks at the end of a program, because all
>   of the program's space is given back to the system when the process
>   terminates.
> @@ -1935,6 +1941,9 @@ linking against @code{libc.a} (explicitly or implicitly).
>   functions (that is, all the functions used by the application,
>   @theglibc{}, and other linked-in libraries) can lead to static linking
>   failures, and, at run time, to heap corruption and application crashes.
> +Replacement functions should implement the behavior documented for
> +their counterparts in @theglibc{}; for example, the replacement
> +@code{free} should also preserve @code{errno}.
>   
>   The minimum set of functions which has to be provided by a custom
>   @code{malloc} is given in the table below.
>