diff mbox series

[v3] stdlib: Fix qsort memory leak if callback throws (BZ 32058)

Message ID 20240808185232.1351850-1-adhemerval.zanella@linaro.org
State New
Headers show
Series [v3] stdlib: Fix qsort memory leak if callback throws (BZ 32058) | expand

Commit Message

Adhemerval Zanella Aug. 8, 2024, 6:51 p.m. UTC 
If the input buffer exceeds the stack auxiliary buffer, qsort will
malloc a temporary one to call mergesort.  Since C++ standard does
allow the callback comparison function to throw [1], the glibc
implementation can potentially leak memory.

The fixes uses a pthread_cleanup_combined_push and
pthread_cleanup_combined_pop, so it can work with and without
exception enables.  The qsort code path that calls malloc now
requires some extra setup and a call to __pthread_cleanup_push
anmd __pthread_cleanup_pop (which should be ok since they just
setup some buffer state).

Checked on x86_64-linux-gnu.

[1] https://timsong-cpp.github.io/cppwp/n4950/alg.c.library#4
---
 stdlib/Makefile        | 32 ++++++++++++++++-
 stdlib/qsort.c         | 81 ++++++++++++++++++++++++++----------------
 stdlib/tst-qsort4.c    |  4 +++
 stdlib/tst-qsort7.c    | 81 ++++++++++++++++++++++++++++++++++++++++++
 stdlib/tst-qsortx7.c   |  1 +
 sysdeps/htl/pthreadP.h |  8 +++++
 6 files changed, 175 insertions(+), 32 deletions(-)
 create mode 100644 stdlib/tst-qsort7.c
 create mode 100644 stdlib/tst-qsortx7.c
diff mbox series

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 347491de53..b68401bd54 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -290,6 +290,8 @@  tests := \
   tst-qsort2 \
   tst-qsort3 \
   tst-qsort6 \
+  tst-qsort7 \
+  tst-qsortx7 \
   tst-quick_exit \
   tst-rand48 \
   tst-rand48-2 \
@@ -539,7 +541,19 @@  tests-special += $(objpfx)isomac.out
 
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-fmtmsg.out
-endif
+ifeq ($(build-shared),yes)
+ifneq ($(PERL),no)
+generated += \
+  tst-qsort7.mtrace \
+  tst-qsortx7.mtrace \
+  # generated
+tests-special += \
+  $(objpfx)tst-qsort7-mem.out \
+  $(objpfx)tst-qsortx7-mem.out \
+  # tests-special
+endif # $(build-shared) == yes
+endif # $(PERL) == yes
+endif # $(run-built-tests) == yes
 
 include ../Rules
 
@@ -627,3 +641,19 @@  $(objpfx)tst-setcontext3.out: tst-setcontext3.sh $(objpfx)tst-setcontext3
 $(objpfx)tst-qsort5: $(libm)
 $(objpfx)tst-concurrent-exit: $(shared-thread-library)
 $(objpfx)tst-concurrent-quick_exit: $(shared-thread-library)
+
+CFLAGS-tst-qsort7.c += -fno-exceptions -fno-asynchronous-unwind-tables
+LDLIBS-tst-qsort7 = $(shared-thread-library)
+tst-qsort7-ENV = MALLOC_TRACE=$(objpfx)tst-qsort7.mtrace \
+		 LD_PRELOAD=$(common-objpfx)/malloc/libc_malloc_debug.so
+$(objpfx)tst-qsort7-mem.out: $(objpfx)tst-qsort7.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-qsort7.mtrace > $@; \
+	$(evaluate-test)
+
+CFLAGS-tst-qsortx7.c += -fexceptions
+LDLIBS-tst-qsortx7 = $(shared-thread-library)
+tst-qsortx7-ENV = MALLOC_TRACE=$(objpfx)tst-qsortx7.mtrace \
+		  LD_PRELOAD=$(common-objpfx)/malloc/libc_malloc_debug.so
+$(objpfx)tst-qsortx7-mem.out: $(objpfx)tst-qsortx7.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-qsortx7.mtrace > $@; \
+	$(evaluate-test)
diff --git a/stdlib/qsort.c b/stdlib/qsort.c
index be47aebbe0..163614c4b8 100644
--- a/stdlib/qsort.c
+++ b/stdlib/qsort.c
@@ -25,6 +25,7 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <stdbool.h>
+#include "pthreadP.h"
 
 /* Swap SIZE bytes between addresses A and B.  These helpers are provided
    along the generic one as an optimization.  */
@@ -338,36 +339,10 @@  indirect_msort_with_tmp (const struct msort_param *p, void *b, size_t n,
       }
 }
 
-void
-__qsort_r (void *const pbase, size_t total_elems, size_t size,
-	   __compar_d_fn_t cmp, void *arg)
+static void
+qsort_r_mergesort (void *const pbase, size_t total_elems, size_t size,
+		   __compar_d_fn_t cmp, void *arg, void *buf)
 {
-  if (total_elems <= 1)
-    return;
-
-  /* Align to the maximum size used by the swap optimization.  */
-  _Alignas (uint64_t) char tmp[QSORT_STACK_SIZE];
-  size_t total_size = total_elems * size;
-  char *buf;
-
-  if (size > INDIRECT_SORT_SIZE_THRES)
-    total_size = 2 * total_elems * sizeof (void *) + size;
-
-  if (total_size <= sizeof tmp)
-    buf = tmp;
-  else
-    {
-      int save = errno;
-      buf = malloc (total_size);
-      __set_errno (save);
-      if (buf == NULL)
-	{
-	  /* Fallback to heapsort in case of memory failure.  */
-	  heapsort_r (pbase, total_elems - 1, size, cmp, arg);
-	  return;
-	}
-    }
-
   if (size > INDIRECT_SORT_SIZE_THRES)
     {
       const struct msort_param msort_param =
@@ -392,9 +367,53 @@  __qsort_r (void *const pbase, size_t total_elems, size_t size,
 	};
       msort_with_tmp (&msort_param, pbase, total_elems);
     }
+}
+
+static bool
+qsort_r_malloc (void *const pbase, size_t total_elems, size_t size,
+		__compar_d_fn_t cmp, void *arg, size_t total_size)
+{
+  int save = errno;
+  char *buf = malloc (total_size);
+  __set_errno (save);
+  if (buf == NULL)
+    return false;
 
-  if (buf != tmp)
-    free (buf);
+  /* Deallocate the auxiliary buffer if the callback function throws
+     or if the thread is cancelled.  */
+  pthread_cleanup_combined_push (free, buf);
+  qsort_r_mergesort (pbase, total_elems, size, cmp, arg, buf);
+  pthread_cleanup_combined_pop (0);
+
+  free (buf);
+
+  return true;
+}
+
+void
+__qsort_r (void *const pbase, size_t total_elems, size_t size,
+	   __compar_d_fn_t cmp, void *arg)
+{
+  if (total_elems <= 1)
+    return;
+
+  /* Align to the maximum size used by the swap optimization.  */
+  size_t total_size = total_elems * size;
+
+  if (size > INDIRECT_SORT_SIZE_THRES)
+    total_size = 2 * total_elems * sizeof (void *) + size;
+
+  if (total_size <= QSORT_STACK_SIZE)
+    {
+      _Alignas (uint64_t) char tmp[QSORT_STACK_SIZE];
+      qsort_r_mergesort (pbase, total_elems, size, cmp, arg, tmp);
+    }
+  else
+    {
+      if (!qsort_r_malloc (pbase, total_elems, size, cmp, arg, total_size))
+	/* Fallback to heapsort in case of memory failure.  */
+	heapsort_r (pbase, total_elems - 1, size, cmp, arg);
+    }
 }
 libc_hidden_def (__qsort_r)
 weak_alias (__qsort_r, qsort_r)
diff --git a/stdlib/tst-qsort4.c b/stdlib/tst-qsort4.c
index 247917b454..b723fa4aab 100644
--- a/stdlib/tst-qsort4.c
+++ b/stdlib/tst-qsort4.c
@@ -16,6 +16,10 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#undef pthread_cleanup_combined_push
+#define pthread_cleanup_combined_push(routine, arg)
+#undef pthread_cleanup_combined_pop
+#define pthread_cleanup_combined_pop(execute)
 #include "qsort.c"
 
 #include <stdio.h>
diff --git a/stdlib/tst-qsort7.c b/stdlib/tst-qsort7.c
new file mode 100644
index 0000000000..ba0c3d7387
--- /dev/null
+++ b/stdlib/tst-qsort7.c
@@ -0,0 +1,81 @@ 
+/* Test if qsort cleanup memory allocation if the comparison function
+   throws (BZ 32058)
+   Copyright (C) 2024 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <mcheck.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/xthread.h>
+#include <unistd.h>
+
+static pthread_barrier_t b;
+
+static void
+cl (void *arg)
+{
+}
+
+static int
+compar_func (const void *a1, const void *a2)
+{
+  xpthread_barrier_wait (&b);
+
+  pthread_cleanup_push (cl, NULL);
+
+  pause ();
+
+  pthread_cleanup_pop (0);
+
+  support_record_failure ();
+
+  return 0;
+}
+
+static void *
+tf (void *tf)
+{
+  /* An array larger than QSORT_STACK_SIZE to force memory allocation.  */
+  int input[1024] = { 0 };
+  qsort (input, array_length (input), sizeof input[0], compar_func);
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  mtrace ();
+
+  xpthread_barrier_init (&b, NULL, 2);
+
+  pthread_t thr = xpthread_create (NULL, tf, NULL);
+
+  xpthread_barrier_wait (&b);
+
+  xpthread_cancel (thr);
+
+  {
+    void *r = xpthread_join (thr);
+    TEST_VERIFY (r == PTHREAD_CANCELED);
+  }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/stdlib/tst-qsortx7.c b/stdlib/tst-qsortx7.c
new file mode 100644
index 0000000000..ab6152320c
--- /dev/null
+++ b/stdlib/tst-qsortx7.c
@@ -0,0 +1 @@ 
+#include "tst-qsort7.c"
diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h
index cf8a2efe86..ef1fa8ca95 100644
--- a/sysdeps/htl/pthreadP.h
+++ b/sysdeps/htl/pthreadP.h
@@ -23,6 +23,7 @@ 
 
 #include <pthread.h>
 #include <link.h>
+#include <bits/cancelation.h>
 
 /* Attribute to indicate thread creation was issued from C11 thrd_create.  */
 #define ATTR_C11_THREAD ((void*)(uintptr_t)-1)
@@ -113,4 +114,11 @@  hidden_proto (__pthread_get_cleanup_stack)
   _Static_assert (sizeof (type) == size,				\
 		  "sizeof (" #type ") != " #size)
 
+#ifndef pthread_cleanup_combined_push
+# define pthread_cleanup_combined_push  __pthread_cleanup_push
+#endif
+#ifndef pthread_cleanup_combined_pop
+# define pthread_cleanup_combined_pop   __pthread_cleanup_pop
+#endif
+
 #endif	/* pthreadP.h */