[PATCH v3] stdlib: Fix qsort memory leak if callback throws (BZ 32058)
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Fri Mar 21 19:49:44 GMT 2025
Ping.
On 08/08/24 15:51, Adhemerval Zanella wrote:
> 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 --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 */
More information about the Libc-alpha
mailing list