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

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

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


[-- Attachment #2: 0001-free-preserve-errno-BZ-17924.patch --]
[-- Type: text/x-patch, Size: 10573 bytes --]

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.
---
 malloc/Makefile         |   1 +
 malloc/malloc.c         |  13 +++-
 malloc/tst-free-errno.c | 167 ++++++++++++++++++++++++++++++++++++++++
 manual/memory.texi      |   9 +++
 4 files changed, 186 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 326075e704..44020d1fd8 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3124,6 +3124,8 @@ __libc_free (void *mem)
   if (mem == 0)                              /* free(0) has no effect */
     return;
 
+  int err = errno;
+
   p = mem2chunk (mem);
 
   if (chunk_is_mmapped (p))                       /* release mmapped memory. */
@@ -3141,13 +3143,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..960c419512
--- /dev/null
+++ b/malloc/tst-free-errno.c
@@ -0,0 +1,167 @@
+/* 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
+
+#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.
-- 
2.29.2