[PATCH] free: preserve errno [BZ#17924]
Adhemerval Zanella
adhemerval.zanella@linaro.org
Mon Dec 28 19:24:29 GMT 2020
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.
More information about the Libc-alpha
mailing list