diff mbox series

free: preserve errno [BZ#17924]

Message ID 20201220202556.3714-1-eggert@cs.ucla.edu
State Superseded
Headers show
Series free: preserve errno [BZ#17924] | expand

Commit Message

Paul Eggert Dec. 20, 2020, 8:25 p.m. UTC
* 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

Comments

Siddhesh Poyarekar Dec. 21, 2020, 2:03 a.m. UTC | #1
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.
>
Carlos O'Donell Dec. 21, 2020, 4:27 a.m. UTC | #2
On 12/20/20 3:25 PM, 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.

Please provide a reasonable commit message.
Paul Eggert Dec. 21, 2020, 7:20 a.m. UTC | #3
On 12/20/20 8:27 PM, Carlos O'Donell wrote:
> Please provide a reasonable commit message.

Thanks, unfortunately I slipped up and used the old-style commits still 
common in other projects. Revised patch attached. This revision also 
addresses Siddhesh's comments (he found another way errno could be munged).
Siddhesh Poyarekar Dec. 21, 2020, 7:43 a.m. UTC | #4
On 12/21/20 12:50 PM, Paul Eggert wrote:
> From e4fec6f28270c9b0979f9fe8920dc52f8f7d70cd Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 19 Dec 2020 12:52:09 -0800
> Subject: [PATCH] free: preserve errno [BZ#17924]
> 
> In the next release of POSIX, free must preserve errno
> <https://www.austingroupbugs.net/view.php?id=385>.
> Modify __libc_free to save and restore errno, so that
> any internal munmap etc. syscalls do not disturb the caller's errno.
> Add a test malloc/tst-free-errno.c (almost all by Bruno Haible),
> and document that free preserves errno.

Thanks, this version looks good to me.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Florian Weimer Dec. 21, 2020, 9:33 a.m. UTC | #5
* Paul Eggert:

> +/* 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)

Long line (80 characters).

I think you need to use this:

int __attribute__ ((weak))
get_errno (void)
{
  return errno;
}

With that volatile construct, the read of errno is not actually behind a
compiler barrier.

Thanks,
Florian
Siddhesh Poyarekar Dec. 21, 2020, 10:03 a.m. UTC | #6
On 12/21/20 3:03 PM, Florian Weimer via Libc-alpha wrote:
> * Paul Eggert:
> 
>> +/* 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)
> 
> Long line (80 characters).
> 
> I think you need to use this:
> 
> int __attribute__ ((weak))
> get_errno (void)
> {
>    return errno;
> }
> 
> With that volatile construct, the read of errno is not actually behind a
> compiler barrier.

Wouldn't it be a bug in the compiler if it delays load of the TLS var 
beyond a function call boundary?

Siddhesh
Siddhesh Poyarekar Dec. 21, 2020, 10:05 a.m. UTC | #7
On 12/21/20 3:33 PM, Siddhesh Poyarekar wrote:
> Wouldn't it be a bug in the compiler if it delays load of the TLS var 
> beyond a function call boundary?

Sorry scratch that, I wasn't thinking straight.

Siddhesh
Paul Eggert Dec. 23, 2020, 5:30 a.m. UTC | #8
Thanks for the comment about the test case's compiler barrier, and about the 
test case's too-long lines. I fixed those, and also fixed the BZ# in the 
Subject line. Revised patch attached.
Adhemerval Zanella Netto Dec. 23, 2020, 7:19 p.m. UTC | #9
On 23/12/2020 02:30, Paul Eggert wrote:
> Thanks for the comment about the test case's compiler barrier, and about the test case's too-long lines. I fixed those, and also fixed the BZ# in the Subject line. Revised patch attached.

> From a994c467ce04abfe102809812156cb30810eaa95 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 22 Dec 2020 21:17:20 -0800
> Subject: [PATCH] free: preserve errno [BZ#17924]
> 
> In the next release of POSIX, free must preserve errno
> <https://www.austingroupbugs.net/view.php?id=385>.
> Modify __libc_free to save and restore errno, so that
> any internal munmap etc. syscalls do not disturb the caller's errno.
> Add a test malloc/tst-free-errno.c (almost all by Bruno Haible),
> and document that free preserves errno.
> ---
>  malloc/Makefile         |   1 +
>  malloc/malloc.c         |  13 +++-
>  malloc/tst-free-errno.c | 168 ++++++++++++++++++++++++++++++++++++++++
>  manual/memory.texi      |   9 +++
>  4 files changed, 187 insertions(+), 4 deletions(-)
>  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 a3e914fa8a..3b151f44f7 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3278,6 +3278,8 @@ __libc_free (void *mem)
>    *(volatile char *)mem;
>  #endif
>  
> +  int err = errno;
> +
>    p = mem2chunk (mem);
>  
>    /* Mark the chunk as belonging to the library again.  */
> @@ -3298,13 +3300,16 @@ __libc_free (void *mem)
>                        mp_.mmap_threshold, mp_.trim_threshold);
>          }
>        munmap_chunk (p);
> -      return;
>      }
> +  else
> +    {
> +      MAYBE_INIT_TCACHE ();
>  
> -  MAYBE_INIT_TCACHE ();
> +      ar_ptr = arena_for_chunk (p);
> +      _int_free (ar_ptr, p, 0);
> +    }
>  
> -  ar_ptr = arena_for_chunk (p);
> -  _int_free (ar_ptr, p, 0);
> +  __set_errno (err);
>  }
>  libc_hidden_def (__libc_free)
>  

I am not very found on adding a errno set/restore on *every* free, specially
because it is extra overhead on small allocations that won't touch mmap/munmap
(specially for tcache case).

Fixing in a more fine grained would require a lot of more work to check if
the shared routines that calls mmap, madvise or brk won't interfere with other
symbols; so maybe it should be ok to use this large hammer for now.

> diff --git a/malloc/tst-free-errno.c b/malloc/tst-free-errno.c
> new file mode 100644
> index 0000000000..a612bbb3bf
> --- /dev/null
> +++ b/malloc/tst-free-errno.c
> @@ -0,0 +1,168 @@
> +/* 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/>.  */
> +
> +#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

Why do you need to include this solely for Linux? I think all of the headers
should be provided by glibc on all supported systems.

> +
> +#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;                         \
> +  }

Why can't you use support/check.h instead?

> +
> +/* The __attribute__ ((weak)) prevents a GCC optimization.  Without
> +   it, GCC would "know" that errno is unchanged by calling free (ptr),
> +   when ptr was the result of a malloc call in the same function.  */
> +int __attribute__ ((weak))
> +get_errno (void)
> +{
> +  return errno;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Check that free() preserves errno.  */
> +  {
> +    errno = 1789; /* Liberté, égalité, fraternité.  */
> +    free (NULL);
> +    ASSERT_NO_STDIO (get_errno () == 1789);
> +  }
> +  { /* Large memory allocations.  */

Maybe add a comment that it is forcing a mmap with the large allocation.

> +    #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 () == 1789);
> +      }
> +    #undef N
> +  }

No need to set/unset N here.

> +
> +  /* 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).

I think there is no need to reference old and unsupported kernel such as
2.4.

> +   */
> +  #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);

Use xmalloc here, so there is no need to check if the allocation has succeded
(they are small enough we can assume a failure means something wrong).

> +      /* 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.  */

Use xmmap.

> +      int fd = open ("/etc/hosts", O_RDONLY);
> +

I think it is better to use a temporary file from support/create_temp_file.h.

> +      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);

User xmalloc.

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

Use xmmap.

> +            {
> +              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 () == 1789);
> +            }
> +        }
> +    }
> +  #endif
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.

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

Not sure if this is worth to add, since we will need to update the manual
once the POSIX does require it.

> @@ -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.
> -- 
> 2.29.2
Paul Eggert Dec. 24, 2020, 1:03 a.m. UTC | #10
Thanks for the comments about the patch's test case. I modified the test 
case to reflect nearly all the comments, resulting in the attached revised 
patch. I'm replying below only to the comments that didn't result in a 
change to the patch.

On 12/23/20 11:19 AM, Adhemerval Zanella wrote:

> Fixing in a more fine grained would require a lot of more work to check if
> the shared routines that calls mmap, madvise or brk won't interfere with other
> symbols; so maybe it should be ok to use this large hammer for now.

Yes, that was my thought as well.

>> +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.
...
> Not sure if this is worth to add, since we will need to update the manual
> once the POSIX does require it.

I'll volunteer to update the manual. :-)

It's worth mentioning that preserving errno is not something that portable C 
or POSIX code should assume for 'free'. If there's a better way for the 
manual to warn its readers about this, that'd be fine of course.

I did consider changing "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" to "Neither @w{ISO C} nor POSIX requires @code{free} 
to preserve @code{errno}", but that wording would be less informative and 
would still need updating once POSIX does require 'free' to preserve errno.
Adhemerval Zanella Netto Dec. 28, 2020, 7:24 p.m. UTC | #11
On 23/12/2020 22:03, Paul Eggert wrote:
> Thanks for the comments about the patch's test case. I modified the test case to reflect nearly all the comments, resulting in the attached revised patch. I'm replying below only to the comments that didn't result in a change to the patch.
> 
> On 12/23/20 11:19 AM, Adhemerval Zanella wrote:
> 
>> Fixing in a more fine grained would require a lot of more work to check if
>> the shared routines that calls mmap, madvise or brk won't interfere with other
>> symbols; so maybe it should be ok to use this large hammer for now.
> 
> Yes, that was my thought as well.

I will try to revise this for 2.33.

> 
>>> +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.
> ...
>> Not sure if this is worth to add, since we will need to update the manual
>> once the POSIX does require it.
> 
> I'll volunteer to update the manual. :-)
> 
> It's worth mentioning that preserving errno is not something that portable C or POSIX code should assume for 'free'. If there's a better way for the manual to warn its readers about this, that'd be fine of course.
> 
> I did consider changing "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" to "Neither @w{ISO C} nor POSIX requires @code{free} to preserve @code{errno}", but that wording would be less informative and would still need updating once POSIX does require 'free' to preserve errno.

Thanks.

> From afbf4ff042cf3a5c8f983d5aa3bd0de3fb696dd3 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 23 Dec 2020 11:27:25 -0800
> Subject: [PATCH] free: preserve errno [BZ#17924]
> 
> In the next release of POSIX, free must preserve errno
> <https://www.austingroupbugs.net/view.php?id=385>.
> Modify __libc_free to save and restore errno, so that
> any internal munmap etc. syscalls do not disturb the caller's errno.
> Add a test malloc/tst-free-errno.c (almost all by Bruno Haible),
> and document that free preserves errno.

LGTM with the small test change below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  malloc/Makefile         |   1 +
>  malloc/malloc.c         |  13 ++--
>  malloc/tst-free-errno.c | 131 ++++++++++++++++++++++++++++++++++++++++
>  manual/memory.texi      |   9 +++
>  4 files changed, 150 insertions(+), 4 deletions(-)
>  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 a3e914fa8a..3b151f44f7 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3278,6 +3278,8 @@ __libc_free (void *mem)
>    *(volatile char *)mem;
>  #endif
>  
> +  int err = errno;
> +
>    p = mem2chunk (mem);
>  
>    /* Mark the chunk as belonging to the library again.  */
> @@ -3298,13 +3300,16 @@ __libc_free (void *mem)
>                        mp_.mmap_threshold, mp_.trim_threshold);
>          }
>        munmap_chunk (p);
> -      return;
>      }
> +  else
> +    {
> +      MAYBE_INIT_TCACHE ();
>  
> -  MAYBE_INIT_TCACHE ();
> +      ar_ptr = arena_for_chunk (p);
> +      _int_free (ar_ptr, p, 0);
> +    }
>  
> -  ar_ptr = arena_for_chunk (p);
> -  _int_free (ar_ptr, p, 0);
> +  __set_errno (err);
>  }
>  libc_hidden_def (__libc_free)
>  
> diff --git a/malloc/tst-free-errno.c b/malloc/tst-free-errno.c
> new file mode 100644
> index 0000000000..89629751e8
> --- /dev/null
> +++ b/malloc/tst-free-errno.c
> @@ -0,0 +1,131 @@
> +/* 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/>.  */
> +
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xunistd.h>
> +
> +/* The __attribute__ ((weak)) prevents a GCC optimization.  Without
> +   it, GCC would "know" that errno is unchanged by calling free (ptr),
> +   when ptr was the result of a malloc call in the same function.  */
> +int __attribute__ ((weak))
> +get_errno (void)
> +{
> +  return errno;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Check that free() preserves errno.  */
> +  {
> +    errno = 1789; /* Liberté, égalité, fraternité.  */
> +    free (NULL);
> +    TEST_VERIFY (get_errno () == 1789);
> +  }
> +  { /* Large memory allocations, to force mmap.  */
> +    enum { N = 2 };
> +    void * volatile ptrs[N];
> +    size_t i;
> +    for (i = 0; i < N; i++)
> +      ptrs[i] = malloc (5318153);

Use xmalloc here.

> +    for (i = 0; i < N; i++)
> +      {
> +        errno = 1789;
> +        free (ptrs[i]);
> +        TEST_VERIFY (get_errno () == 1789);
> +      }
> +  }
> +
> +  /* 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.  For all known Linux versions
> +     the default limit is at most 65536.
> +   */
> +  #if defined __linux__
> +  if (open ("/proc/sys/vm/max_map_count", O_RDONLY) >= 0)

I think we can assume for tests /proc should be mounted, otherwise this
only partially test this interface.  So I think we can use xopen here.

> +    {
> +      /* Preparations.  */
> +      size_t pagesize = getpagesize ();
> +      void *firstpage_backup = xmalloc (pagesize);
> +      void *lastpage_backup = xmalloc (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.  */
> +      xmmap (NULL, 0x1000000, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1);
> +      /* A file descriptor pointing to a regular file.  */
> +      int fd = create_temp_file ("tst-free-errno", NULL);
> +      if (fd < 0)
> +	FAIL_EXIT1 ("cannot create temporary file");
> +
> +      /* Do a large memory allocation.  */
> +      size_t big_size = 0x1000000;
> +      void * volatile ptr = xmalloc (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);
> +      xmmap (ptr_aligned - pagesize, pagesize + big_size + pagesize,
> +	     PROT_READ | PROT_WRITE,
> +	     MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -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).  */
> +      for (int i = 0; i < 65536; i++)
> +	if (mmap (NULL, pagesize, PROT_READ, MAP_FILE | MAP_PRIVATE, fd, 0)
> +	    == MAP_FAILED)
> +	  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);
> +      TEST_VERIFY (get_errno () == 1789);
> +    }
> +  #endif
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.

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

Ok.

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

Ok.
H.J. Lu Dec. 29, 2020, 1:38 p.m. UTC | #12
On Mon, Dec 28, 2020 at 11:24 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 23/12/2020 22:03, Paul Eggert wrote:
> > Thanks for the comments about the patch's test case. I modified the test case to reflect nearly all the comments, resulting in the attached revised patch. I'm replying below only to the comments that didn't result in a change to the patch.
> >
> > On 12/23/20 11:19 AM, Adhemerval Zanella wrote:
> >
> >> Fixing in a more fine grained would require a lot of more work to check if
> >> the shared routines that calls mmap, madvise or brk won't interfere with other
> >> symbols; so maybe it should be ok to use this large hammer for now.
> >
> > Yes, that was my thought as well.
>
> I will try to revise this for 2.33.
>
> >
> >>> +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.
> > ...
> >> Not sure if this is worth to add, since we will need to update the manual
> >> once the POSIX does require it.
> >
> > I'll volunteer to update the manual. :-)
> >
> > It's worth mentioning that preserving errno is not something that portable C or POSIX code should assume for 'free'. If there's a better way for the manual to warn its readers about this, that'd be fine of course.
> >
> > I did consider changing "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" to "Neither @w{ISO C} nor POSIX requires @code{free} to preserve @code{errno}", but that wording would be less informative and would still need updating once POSIX does require 'free' to preserve errno.
>
> Thanks.
>
> > From afbf4ff042cf3a5c8f983d5aa3bd0de3fb696dd3 Mon Sep 17 00:00:00 2001
> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Wed, 23 Dec 2020 11:27:25 -0800
> > Subject: [PATCH] free: preserve errno [BZ#17924]
> >
> > In the next release of POSIX, free must preserve errno
> > <https://www.austingroupbugs.net/view.php?id=385>.
> > Modify __libc_free to save and restore errno, so that
> > any internal munmap etc. syscalls do not disturb the caller's errno.
> > Add a test malloc/tst-free-errno.c (almost all by Bruno Haible),
> > and document that free preserves errno.
>
> LGTM with the small test change below.
>
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>

On a machine with 192 GB RAM, I got

[hjl@gnu-clx-1 build-x86_64-linux]$ cat malloc/tst-free-errno-mcheck.out
error: tst-free-errno.c:124: not true: get_errno () == 1789
error: 1 test failures
[hjl@gnu-clx-1 build-x86_64-linux]$
Adhemerval Zanella Netto Dec. 29, 2020, 6:32 p.m. UTC | #13
On 29/12/2020 10:38, H.J. Lu wrote:
> On Mon, Dec 28, 2020 at 11:24 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>>
>>
>> On 23/12/2020 22:03, Paul Eggert wrote:
>>> Thanks for the comments about the patch's test case. I modified the test case to reflect nearly all the comments, resulting in the attached revised patch. I'm replying below only to the comments that didn't result in a change to the patch.
>>>
>>> On 12/23/20 11:19 AM, Adhemerval Zanella wrote:
>>>
>>>> Fixing in a more fine grained would require a lot of more work to check if
>>>> the shared routines that calls mmap, madvise or brk won't interfere with other
>>>> symbols; so maybe it should be ok to use this large hammer for now.
>>>
>>> Yes, that was my thought as well.
>>
>> I will try to revise this for 2.33.
>>
>>>
>>>>> +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.
>>> ...
>>>> Not sure if this is worth to add, since we will need to update the manual
>>>> once the POSIX does require it.
>>>
>>> I'll volunteer to update the manual. :-)
>>>
>>> It's worth mentioning that preserving errno is not something that portable C or POSIX code should assume for 'free'. If there's a better way for the manual to warn its readers about this, that'd be fine of course.
>>>
>>> I did consider changing "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" to "Neither @w{ISO C} nor POSIX requires @code{free} to preserve @code{errno}", but that wording would be less informative and would still need updating once POSIX does require 'free' to preserve errno.
>>
>> Thanks.
>>
>>> From afbf4ff042cf3a5c8f983d5aa3bd0de3fb696dd3 Mon Sep 17 00:00:00 2001
>>> From: Paul Eggert <eggert@cs.ucla.edu>
>>> Date: Wed, 23 Dec 2020 11:27:25 -0800
>>> Subject: [PATCH] free: preserve errno [BZ#17924]
>>>
>>> In the next release of POSIX, free must preserve errno
>>> <https://www.austingroupbugs.net/view.php?id=385>.
>>> Modify __libc_free to save and restore errno, so that
>>> any internal munmap etc. syscalls do not disturb the caller's errno.
>>> Add a test malloc/tst-free-errno.c (almost all by Bruno Haible),
>>> and document that free preserves errno.
>>
>> LGTM with the small test change below.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
> 
> On a machine with 192 GB RAM, I got
> 
> [hjl@gnu-clx-1 build-x86_64-linux]$ cat malloc/tst-free-errno-mcheck.out
> error: tst-free-errno.c:124: not true: get_errno () == 1789
> error: 1 test failures
> [hjl@gnu-clx-1 build-x86_64-linux]$
> 

We need to fix it for the malloc check hooks as well:

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 6474ba8b38..336ff497e9 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -260,6 +260,8 @@ free_check (void *mem, const void *caller)
   if (!mem)
     return;
 
+  int err = errno;
+
 #ifdef USE_MTAG
   /* Quickly check that the freed pointer matches the tag for the memory.
      This gives a useful double-free detection.  */
@@ -274,12 +276,16 @@ free_check (void *mem, const void *caller)
     {
       __libc_lock_unlock (main_arena.mutex);
       munmap_chunk (p);
-      return;
     }
-  /* Mark the chunk as belonging to the library again.  */
-  (void)TAG_REGION (chunk2rawmem (p), CHUNK_AVAILABLE_SIZE (p) - CHUNK_HDR_SZ);
-  _int_free (&main_arena, p, 1);
-  __libc_lock_unlock (main_arena.mutex);
+  else
+    {
+      /* Mark the chunk as belonging to the library again.  */
+      (void)TAG_REGION (chunk2rawmem (p), CHUNK_AVAILABLE_SIZE (p)
+                                         - CHUNK_HDR_SZ);
+      _int_free (&main_arena, p, 1);
+      __libc_lock_unlock (main_arena.mutex);
+    }
+  __set_errno (err);
 }
 
 static void *
diff mbox series

Patch

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;
     }
 
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.  */
+
+#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.