diff mbox series

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

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

Commit Message

Adhemerval Zanella Aug. 8, 2024, 2:17 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 now requires some extra setup and
a call to __pthread_cleanup_push/__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         | 26 +++++++++++---
 stdlib/tst-qsort4.c    |  4 +++
 stdlib/tst-qsort7.c    | 81 ++++++++++++++++++++++++++++++++++++++++++
 stdlib/tst-qsortx7.c   |  1 +
 sysdeps/htl/pthreadP.h |  8 +++++
 6 files changed, 147 insertions(+), 5 deletions(-)
 create mode 100644 stdlib/tst-qsort7.c
 create mode 100644 stdlib/tst-qsortx7.c

Comments

Andreas Schwab Aug. 8, 2024, 2:31 p.m. UTC | #1 
On Aug 08 2024, Adhemerval Zanella wrote:

> +  /* This check for NULL helps the compiler to that is does not generate
                                               so that it
Florian Weimer Aug. 8, 2024, 4:44 p.m. UTC | #2 
* Adhemerval Zanella:

> 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 now requires some extra setup and
> a call to __pthread_cleanup_push/__pthread_cleanup_pop (which
> should be ok since they just setup some buffer state).

Can we restrict this to the path that actually performs malloc?

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

Ideally this should be in a separate commit, in case we need to backport
this indepedently.

Thanks,
Florian
Adhemerval Zanella Aug. 8, 2024, 6:13 p.m. UTC | #3 
On 08/08/24 13:44, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> 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 now requires some extra setup and
>> a call to __pthread_cleanup_push/__pthread_cleanup_pop (which
>> should be ok since they just setup some buffer state).
> 
> Can we restrict this to the path that actually performs malloc?

I tried to make this patch as simple as possible, but I think it should
double.

> 
>> 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_push   __pthread_cleanup_pop
>> +#endif
>> +
>>  #endif	/* pthreadP.h */
> 
> Ideally this should be in a separate commit, in case we need to backport
> this indepedently.

This is required for Hurd because pthread_cleanup_combined_push / pthread_cleanup_combined_push
was only added for Linux and I don't really want to spend time trying to make it
work on Hurd.
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..a0feda1236 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,6 +339,17 @@  indirect_msort_with_tmp (const struct msort_param *p, void *b, size_t n,
       }
 }
 
+static void
+__attribute_used__
+cancel_handler (void *ptr)
+{
+  void *mem = *(void **) ptr;
+  /* This check for NULL helps the compiler to that is does not generate
+     explicit calls for free (NULL).  */
+  if (mem != NULL)
+    free (mem);
+}
+
 void
 __qsort_r (void *const pbase, size_t total_elems, size_t size,
 	   __compar_d_fn_t cmp, void *arg)
@@ -349,6 +361,9 @@  __qsort_r (void *const pbase, size_t total_elems, size_t size,
   _Alignas (uint64_t) char tmp[QSORT_STACK_SIZE];
   size_t total_size = total_elems * size;
   char *buf;
+  void *bufmem = NULL;
+
+  pthread_cleanup_combined_push (cancel_handler, &bufmem);
 
   if (size > INDIRECT_SORT_SIZE_THRES)
     total_size = 2 * total_elems * sizeof (void *) + size;
@@ -358,14 +373,15 @@  __qsort_r (void *const pbase, size_t total_elems, size_t size,
   else
     {
       int save = errno;
-      buf = malloc (total_size);
+      bufmem = malloc (total_size);
       __set_errno (save);
-      if (buf == NULL)
+      if (bufmem == NULL)
 	{
 	  /* Fallback to heapsort in case of memory failure.  */
 	  heapsort_r (pbase, total_elems - 1, size, cmp, arg);
 	  return;
 	}
+      buf = bufmem;
     }
 
   if (size > INDIRECT_SORT_SIZE_THRES)
@@ -393,8 +409,10 @@  __qsort_r (void *const pbase, size_t total_elems, size_t size,
       msort_with_tmp (&msort_param, pbase, total_elems);
     }
 
-  if (buf != tmp)
-    free (buf);
+  pthread_cleanup_combined_pop (0);
+
+  if (bufmem != NULL)
+    free (bufmem);
 }
 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 */