[PATCH v6] locale: memory leak in newlocale [BZ #25770]

[Florian Weimer]

* Dmitry Kovalenko:

> diff --git a/locale/Makefile b/locale/Makefile
> index 9d9c1a7691..a9d29d08bc 100644
> --- a/locale/Makefile
> +++ b/locale/Makefile

> @@ -184,3 +195,13 @@ $(objpfx)tst-locale-locpath.out : tst-locale-locpath.sh $(objpfx)locale
>  	$(evaluate-test)
>  
>  $(objpfx)tst-localedef-path-norm: $(shared-thread-library)
> +
> +ifeq ($(run-built-tests),yes)
> +endif # $(run-built-tests) == yes

This appears to be a spurious change.

> +LDLIBS-tst-newlocale = $(shared-thread-library)
> +tst-newlocale-ENV = MALLOC_TRACE=$(objpfx)tst-newlocale.mtrace \
> +		 LD_PRELOAD=$(common-objpfx)/malloc/libc_malloc_debug.so
> +$(objpfx)tst-newlocale-mem.out: $(objpfx)tst-newlocale.out
> +	$(common-objpfx)malloc/mtrace $(objpfx)tst-newlocale.mtrace > $@; \
> +	$(evaluate-test)
> diff --git a/locale/newlocale.c b/locale/newlocale.c
> index d25a6038d3..cc93131fe8 100644
> --- a/locale/newlocale.c
> +++ b/locale/newlocale.c
> @@ -22,6 +22,7 @@
>  #include <locale.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <assert.h>
>  
>  #include "localeinfo.h"
>  
> @@ -38,19 +39,21 @@ __libc_rwlock_define (extern , __libc_setlocale_lock attribute_hidden)
>    } while (0)
>  
>  
> -locale_t
> -__newlocale (int category_mask, const char *locale, locale_t base)
> +static locale_t
> +__newlocale_1 (int category_mask, const char *locale, locale_t base, char ** const locale_path_ptr)
>  {
>    /* Intermediate memory for result.  */
>    const char *newnames[__LC_LAST];
>    struct __locale_struct result;
>    locale_t result_ptr;
> -  char *locale_path;
>    size_t locale_path_len;
>    const char *locpath_var;
>    int cnt;
>    size_t names_len;
>  
> +  assert(locale_path_ptr != NULL);
> +  assert((*locale_path_ptr) == NULL);

Please use ' (' in function-like macro calls, so:

  assert (locale_path_ptr != NULL);
  assert (*locale_path_ptr == NULL);

> +locale_t
> +__newlocale (int category_mask, const char *locale, locale_t base)
> +{
> +  char *tmp_buffer = NULL;
> +
> +  const locale_t result = __newlocale_1(
> +      category_mask,
> +      locale,
> +      base,
> +      &tmp_buffer);
> +
> +  free(tmp_buffer);
> +
> +  return result;
> +}

We generally do not write function calls in this way.  Just list the
arguments on a single line, and wrap as needed.  The first argument on
each line should be at the same column.

(Also missing space before '('.)

I can fix up these style issues for you before pushing on your behalf.



> +
>  weak_alias (__newlocale, newlocale)
> diff --git a/locale/tst-newlocale.c b/locale/tst-newlocale.c
> new file mode 100755
> index 0000000000..fc49eabe84
> --- /dev/null
> +++ b/locale/tst-newlocale.c
> @@ -0,0 +1,55 @@
> +/* This test checks a memory leak in newlocal function [BZ #25770].
> +   Copyright (C) 2025 Free Software Foundation, Inc.

If you are submitting under DCO, this should say:

   Copyright The GNU Toolchain Authors.

You are not assigning copright to the FSF.
  
> +  {
> +    char s[] = "LOCPATH=/usr/share/locale";

As far as I understand it, this path is not actually used.  Is this
accurate?  If so, please add a comment.  Ordinarily, this would be a
test bug because it fails to isolate the test from the host environment.

> +    int const r = putenv (s);
> +
> +    if (r != 0)
> +      {
> +        printf ("putenv failed: %m\n");
> +        exit (EXIT_FAILURE);
> +      }
> +  }

You could include <support/check.h> and use

  if (r != 0)
    FAIL_EXIT1 ("putenv: %m");

> +  {
> +    locale_t const l = newlocale (1 << LC_CTYPE, "POSIX", NULL);
> +
> +    if (l == NULL)
> +      {
> +        printf ("newlocale failed: %m\n");
> +        exit (EXIT_FAILURE);
> +      }

Likewise.

Thanks,
Florian