[PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Florian Weimer]

This fixes a regression introduced in commit
7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
memory leak [BZ #24583]").

(The test depends on the test-in-containers fix for installed locales,
which is currently under discussion.)

2019-07-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #24677]
	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
	counter before freeing array.
	* libio/Makefile (tests-container): Add tst-wfile-fclose-exit.
	* libio/tst-wfile-fclose-exit.c: New file.

diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
index 9a456bf825..1b2500046e 100644
--- a/iconv/gconv_cache.c
+++ b/iconv/gconv_cache.c
@@ -446,9 +446,13 @@ __gconv_lookup_cache (const char *toset, const char *fromset,
 void
 __gconv_release_cache (struct __gconv_step *steps, size_t nsteps)
 {
-  if (gconv_cache != NULL)
-    /* The only thing we have to deallocate is the record with the
-       steps.  */
+  /* The only thing we have to deallocate is the record with the
+     steps. But do not do this if the reference counter is still
+     positive.  This can happen if the steps array was cloned by
+     __wcsmbs_clone_conv.  (The array elements have separate __counter
+     fields, but they are only out of sync temporarily.)  */
+  if (gconv_cache != NULL && steps != NULL && nsteps > 0
+      && steps->__counter == 0)
     free (steps);
 }
 
diff --git a/libio/Makefile b/libio/Makefile
index 6e594b8ec5..3a4fb2f10e 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -68,9 +68,9 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
 	tst-wfile-sync tst-wfile-gconv
 
-# This test tests interaction with the gconv cache.  Setting
-# GCONV_CACHE during out-of-container testing disables the cache.
-tests-container += tst-wfile-ascii
+# These tests interact with the gconv cache.  Setting GCONV_CACHE
+# during out-of-container testing disables the cache.
+tests-container += tst-wfile-ascii tst-wfile-fclose-exit
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -230,6 +230,7 @@ $(objpfx)tst-widetext.out: $(gen-locales)
 $(objpfx)tst_wprintf2.out: $(gen-locales)
 $(objpfx)tst-wfile-sync.out: $(gen-locales)
 $(objpfx)tst-wfile-gconv.out: $(gen-locales)
+$(objpfx)tst-wfile-fclose-exit.out: $(gen-locales)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
diff --git a/libio/tst-wfile-fclose-exit.c b/libio/tst-wfile-fclose-exit.c
new file mode 100644
index 0000000000..9e519fb95e
--- /dev/null
+++ b/libio/tst-wfile-fclose-exit.c
@@ -0,0 +1,70 @@
+/* Check that it is possible to call fclose on default streams.
+   Copyright (C) 2019 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 <locale.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+#include <wchar.h>
+
+static int
+do_test (void)
+{
+  /* The test-in-container framework sets these environment variables.
+     The presence of GCONV_PATH invalidates this test.  */
+  unsetenv ("GCONV_PATH");
+  unsetenv ("LOCPATH");
+
+  /* Create the gconv module cache.  iconvconfig is in /sbin, which is
+     not on PATH.  */
+  {
+    char *iconvconfig = xasprintf ("%s/iconvconfig", support_sbindir_prefix);
+    TEST_COMPARE (system (iconvconfig), 0);
+    free (iconvconfig);
+  }
+
+  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
+    FAIL_EXIT1 ("setlocale: %m");
+
+  char *path;
+  xclose (create_temp_file ("tst-wfile-fclose-exit-", &path));
+  support_write_file_string (path, "test file\n");
+
+  /* Create a stream and put it into wide mode.  */
+  FILE *fp = xfopen (path, "r");
+  wint_t wc = fgetwc (fp);
+  TEST_COMPARE (wc, 't');
+
+  /* Make sure that stdout is fully initialized and in wide mode.  */
+  wprintf (L"info: character read: %d\n", (int) wc);
+
+  xfclose (fp);
+  xfclose (stdin);
+  xfclose (stdout);
+  xfclose (stderr);
+  exit (0);
+
+  free (path);
+
+  return 0;
+}
+
+#include <support/test-driver.c>

Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Yann Droneaud]

Hi,

Le mercredi 10 juillet 2019 à 17:42 +0200, Florian Weimer a écrit :
> This fixes a regression introduced in commit
> 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
> memory leak [BZ #24583]").
> 
> (The test depends on the test-in-containers fix for installed locales,
> which is currently under discussion.)
> 
> 2019-07-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24677]
> 	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
> 	counter before freeing array.
> 	* libio/Makefile (tests-container): Add tst-wfile-fclose-exit.
> 	* libio/tst-wfile-fclose-exit.c: New file.
> 

[...]

> diff --git a/libio/tst-wfile-fclose-exit.c b/libio/tst-wfile-fclose-exit.c
> new file mode 100644
> index 0000000000..9e519fb95e
> --- /dev/null
> +++ b/libio/tst-wfile-fclose-exit.c
> @@ -0,0 +1,70 @@
> +/* Check that it is possible to call fclose on default streams.
> +   Copyright (C) 2019 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 <locale.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +#include <wchar.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* The test-in-container framework sets these environment variables.
> +     The presence of GCONV_PATH invalidates this test.  */
> +  unsetenv ("GCONV_PATH");
> +  unsetenv ("LOCPATH");
> +
> +  /* Create the gconv module cache.  iconvconfig is in /sbin, which is
> +     not on PATH.  */
> +  {
> +    char *iconvconfig = xasprintf ("%s/iconvconfig", support_sbindir_prefix);
> +    TEST_COMPARE (system (iconvconfig), 0);
> +    free (iconvconfig);
> +  }
> +
> +  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
> +    FAIL_EXIT1 ("setlocale: %m");
> +
> +  char *path;
> +  xclose (create_temp_file ("tst-wfile-fclose-exit-", &path));
> +  support_write_file_string (path, "test file\n");
> +
> +  /* Create a stream and put it into wide mode.  */
> +  FILE *fp = xfopen (path, "r");
> +  wint_t wc = fgetwc (fp);
> +  TEST_COMPARE (wc, 't');
> +
> +  /* Make sure that stdout is fully initialized and in wide mode.  */
> +  wprintf (L"info: character read: %d\n", (int) wc);
> +
> +  xfclose (fp);
> +  xfclose (stdin);
> +  xfclose (stdout);
> +  xfclose (stderr);
> +  exit (0);
> +

If exit happen here, why free(path) below ?

> +  free (path);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Regards.

-- 
Yann Droneaud
OPTEYA

[PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Florian Weimer]

This fixes a regression introduced in commit
7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
memory leak [BZ #24583]").

__gconv_release_cache is only ever called with heap-allocated
arrays which contain at least one member.  The statically allocated
ASCII steps are filtered out by __wcsmbs_close_conv.

[This variant cleans up __gconv_release_cache somewhat and includes the
test modification suggested by Yann.

The test does not work due to bug 24824.  I think installing all locales
unconditionally is too costly timewise.  Suggestions how to proceed are
very welcome.]

2019-07-18  Florian Weimer  <fweimer@redhat.com>

	[BZ #24677]
	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
	counter before freeing array.
	* libio/Makefile (tests-container): Add tst-wfile-fclose-exit.
	* libio/tst-wfile-fclose-exit.c: New file.
	* libio/tst-wfile-fclose-exit.root/postclean.req: Likewise.

diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
index 9a456bf825..4db7287cee 100644
--- a/iconv/gconv_cache.c
+++ b/iconv/gconv_cache.c
@@ -446,9 +446,12 @@ __gconv_lookup_cache (const char *toset, const char *fromset,
 void
 __gconv_release_cache (struct __gconv_step *steps, size_t nsteps)
 {
-  if (gconv_cache != NULL)
-    /* The only thing we have to deallocate is the record with the
-       steps.  */
+  /* The only thing we have to deallocate is the record with the
+     steps.  But do not do this if the reference counter is still
+     positive.  This can happen if the steps array was cloned by
+     __wcsmbs_clone_conv.  (The array elements have separate __counter
+     fields, but they are only out of sync temporarily.)  */
+  if (gconv_cache != NULL && steps->__counter == 0)
     free (steps);
 }
 
diff --git a/libio/Makefile b/libio/Makefile
index 6e594b8ec5..3a4fb2f10e 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -68,9 +68,9 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
 	tst-wfile-sync tst-wfile-gconv
 
-# This test tests interaction with the gconv cache.  Setting
-# GCONV_CACHE during out-of-container testing disables the cache.
-tests-container += tst-wfile-ascii
+# These tests interact with the gconv cache.  Setting GCONV_CACHE
+# during out-of-container testing disables the cache.
+tests-container += tst-wfile-ascii tst-wfile-fclose-exit
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -230,6 +230,7 @@ $(objpfx)tst-widetext.out: $(gen-locales)
 $(objpfx)tst_wprintf2.out: $(gen-locales)
 $(objpfx)tst-wfile-sync.out: $(gen-locales)
 $(objpfx)tst-wfile-gconv.out: $(gen-locales)
+$(objpfx)tst-wfile-fclose-exit.out: $(gen-locales)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
diff --git a/libio/tst-wfile-fclose-exit.c b/libio/tst-wfile-fclose-exit.c
new file mode 100644
index 0000000000..e9f19b5155
--- /dev/null
+++ b/libio/tst-wfile-fclose-exit.c
@@ -0,0 +1,73 @@
+/* Check that it is possible to call fclose on wide default streams.
+   Copyright (C) 2019 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 <locale.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+#include <wchar.h>
+
+static int
+do_test (void)
+{
+  /* The test-in-container framework sets these environment variables.
+     The presence of GCONV_PATH invalidates this test.  */
+  unsetenv ("GCONV_PATH");
+  unsetenv ("LOCPATH");
+
+  /* Create the gconv module cache.  iconvconfig is in /sbin, which is
+     not on PATH.  */
+  {
+    char *iconvconfig = xasprintf ("%s/iconvconfig", support_sbindir_prefix);
+    TEST_COMPARE (system (iconvconfig), 0);
+    free (iconvconfig);
+  }
+
+  /* Build the en_US.UTF-8 locale.  */
+  TEST_COMPARE (system ("localedef -f UTF-8 -i en_US en_US.UTF-8"), 0);
+
+  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
+    FAIL_EXIT1 ("setlocale: %m");
+
+  char *path;
+  xclose (create_temp_file ("tst-wfile-fclose-exit-", &path));
+  support_write_file_string (path, "test file\n");
+
+  /* Create a stream and put it into wide mode.  */
+  FILE *fp = xfopen (path, "r");
+  wint_t wc = fgetwc (fp);
+  TEST_COMPARE (wc, 't');
+
+  /* Make sure that stdout is fully initialized and in wide mode.  */
+  wprintf (L"info: character read: %d\n", (int) wc);
+
+  xfclose (fp);
+  xfclose (stdin);
+  xfclose (stdout);
+  xfclose (stderr);
+  free (path);
+
+  exit (0);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/libio/tst-wfile-fclose-exit.root/postclean.req b/libio/tst-wfile-fclose-exit.root/postclean.req
new file mode 100644
index 0000000000..e69de29bb2

Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Carlos O'Donell]

On 7/18/19 12:21 PM, Florian Weimer wrote:
> This fixes a regression introduced in commit
> 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
> memory leak [BZ #24583]").
> 
> __gconv_release_cache is only ever called with heap-allocated
> arrays which contain at least one member.  The statically allocated
> ASCII steps are filtered out by __wcsmbs_close_conv.
> 
> [This variant cleans up __gconv_release_cache somewhat and includes the
> test modification suggested by Yann.
> 
> The test does not work due to bug 24824.  I think installing all locales
> unconditionally is too costly timewise.  Suggestions how to proceed are
> very welcome.]
> 
> 2019-07-18  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24677]
> 	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
> 	counter before freeing array.
> 	* libio/Makefile (tests-container): Add tst-wfile-fclose-exit.
> 	* libio/tst-wfile-fclose-exit.c: New file.
> 	* libio/tst-wfile-fclose-exit.root/postclean.req: Likewise.

Please post a v2 without a test case.

You'll have to confirm this works by hand and I'm OK with that for now.

This was quite complex to audit. The code manipulates a lot of distinct
members of structs without any clear rules of ownership or isolation.
This could all do with some kind of API or cleaner interface to handling
the functions instead of having abstract rules for how the elements are
handled. This was quite a mess. Thank you for the fix!

> diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
> index 9a456bf825..4db7287cee 100644
> --- a/iconv/gconv_cache.c
> +++ b/iconv/gconv_cache.c
> @@ -446,9 +446,12 @@ __gconv_lookup_cache (const char *toset, const char *fromset,
>   void
>   __gconv_release_cache (struct __gconv_step *steps, size_t nsteps)
>   {
> -  if (gconv_cache != NULL)
> -    /* The only thing we have to deallocate is the record with the
> -       steps.  */
> +  /* The only thing we have to deallocate is the record with the
> +     steps.  But do not do this if the reference counter is still
> +     positive.  This can happen if the steps array was cloned by
> +     __wcsmbs_clone_conv.  (The array elements have separate __counter
> +     fields, but they are only out of sync temporarily.)  */
> +  if (gconv_cache != NULL && steps->__counter == 0)
>       free (steps);
>   }

(1) Taking __gconv_lock to protect __counter.

In __wcsmbs_clone_conv we use __gconv_lock to protect __counter.

In __gconv_release_cache we don't use __gconv_lock to protect __counter.

Only __gconv_close_transform calls __gconv_release_cache.

And __gconv_close_transform takes __gconv_lock so accessing __counter is OK.

(2) Distinct __counter fields.

I agree that the elements have distinct counter fields, and I double
checked that they will all be in sync, so that comment looks good, and
the logic makes sense.

Would love to see us make some sensible inline APIs like __*_in_use()
so we could remove all the step->__counter == 0 checks for something
that semantics.

OK.
    
> diff --git a/libio/Makefile b/libio/Makefile
> index 6e594b8ec5..3a4fb2f10e 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -68,9 +68,9 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
>   	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
>   	tst-wfile-sync tst-wfile-gconv
>   
> -# This test tests interaction with the gconv cache.  Setting
> -# GCONV_CACHE during out-of-container testing disables the cache.
> -tests-container += tst-wfile-ascii
> +# These tests interact with the gconv cache.  Setting GCONV_CACHE
> +# during out-of-container testing disables the cache.
> +tests-container += tst-wfile-ascii tst-wfile-fclose-exit

Please remove this.

This test doesn't work because of the converters being in the container
compressed and us not having any way in the container to decompress them.
I think we need a new install target for this so we can avoid the dependency
on gzip/bzip2, or we need to use compression that we bundle.

>   
>   tests-internal = tst-vtables tst-vtables-interposed tst-readline
>   
> @@ -230,6 +230,7 @@ $(objpfx)tst-widetext.out: $(gen-locales)
>   $(objpfx)tst_wprintf2.out: $(gen-locales)
>   $(objpfx)tst-wfile-sync.out: $(gen-locales)
>   $(objpfx)tst-wfile-gconv.out: $(gen-locales)
> +$(objpfx)tst-wfile-fclose-exit.out: $(gen-locales)
>   endif
>   
>   $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
> diff --git a/libio/tst-wfile-fclose-exit.c b/libio/tst-wfile-fclose-exit.c
> new file mode 100644
> index 0000000000..e9f19b5155
> --- /dev/null
> +++ b/libio/tst-wfile-fclose-exit.c

At this point in the release I'd like to see this fixed even without a test
case.

Please remove this test, test manually, and propose a test when 2.31 opens
with the proper fix, which is to install the decompressed converters in
a custom install target for testing (yes this is a deviation from what
we install normally, but it only changes some of the code being tested
e.g. charmap_open() and fopen_uncompressed()).

> @@ -0,0 +1,73 @@
> +/* Check that it is possible to call fclose on wide default streams.
> +   Copyright (C) 2019 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 <locale.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +#include <wchar.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* The test-in-container framework sets these environment variables.
> +     The presence of GCONV_PATH invalidates this test.  */
> +  unsetenv ("GCONV_PATH");
> +  unsetenv ("LOCPATH");
> +
> +  /* Create the gconv module cache.  iconvconfig is in /sbin, which is
> +     not on PATH.  */
> +  {
> +    char *iconvconfig = xasprintf ("%s/iconvconfig", support_sbindir_prefix);
> +    TEST_COMPARE (system (iconvconfig), 0);
> +    free (iconvconfig);
> +  }
> +
> +  /* Build the en_US.UTF-8 locale.  */
> +  TEST_COMPARE (system ("localedef -f UTF-8 -i en_US en_US.UTF-8"), 0);
> +
> +  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
> +    FAIL_EXIT1 ("setlocale: %m");
> +
> +  char *path;
> +  xclose (create_temp_file ("tst-wfile-fclose-exit-", &path));
> +  support_write_file_string (path, "test file\n");
> +
> +  /* Create a stream and put it into wide mode.  */
> +  FILE *fp = xfopen (path, "r");
> +  wint_t wc = fgetwc (fp);
> +  TEST_COMPARE (wc, 't');
> +
> +  /* Make sure that stdout is fully initialized and in wide mode.  */
> +  wprintf (L"info: character read: %d\n", (int) wc);
> +
> +  xfclose (fp);
> +  xfclose (stdin);
> +  xfclose (stdout);
> +  xfclose (stderr);
> +  free (path);
> +
> +  exit (0);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/libio/tst-wfile-fclose-exit.root/postclean.req b/libio/tst-wfile-fclose-exit.root/postclean.req
> new file mode 100644
> index 0000000000..e69de29bb2
> 


-- 
Cheers,
Carlos.

Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Florian Weimer]

* Carlos O'Donell:

> On 7/18/19 12:21 PM, Florian Weimer wrote:
>> This fixes a regression introduced in commit
>> 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
>> memory leak [BZ #24583]").
>>
>> __gconv_release_cache is only ever called with heap-allocated
>> arrays which contain at least one member.  The statically allocated
>> ASCII steps are filtered out by __wcsmbs_close_conv.
>>
>> [This variant cleans up __gconv_release_cache somewhat and includes the
>> test modification suggested by Yann.
>>
>> The test does not work due to bug 24824.  I think installing all locales
>> unconditionally is too costly timewise.  Suggestions how to proceed are
>> very welcome.]
>>
>> 2019-07-18  Florian Weimer  <fweimer@redhat.com>
>>
>> 	[BZ #24677]
>> 	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
>> 	counter before freeing array.
>> 	* libio/Makefile (tests-container): Add tst-wfile-fclose-exit.
>> 	* libio/tst-wfile-fclose-exit.c: New file.
>> 	* libio/tst-wfile-fclose-exit.root/postclean.req: Likewise.
>
> Please post a v2 without a test case.

Thanks.  Please see below.

> You'll have to confirm this works by hand and I'm OK with that for
> now.

valgrind is clean, but __libc_freeres is not fully effective:

==2503== 208 bytes in 1 blocks are still reachable in loss record 1 of 2
==2503==    at 0x480B80B: malloc (vg_replace_malloc.c:309)
==2503==    by 0x4847876: __gconv_lookup_cache (gconv_cache.c:366)
==2503==    by 0x483F9A1: __gconv_find_transform (gconv_db.c:733)
==2503==    by 0x48C8847: __wcsmbs_getfct (wcsmbsload.c:92)
==2503==    by 0x48C89A9: __wcsmbs_load_conv (wcsmbsload.c:186)
==2503==    by 0x48C8C26: get_gconv_fcts (wcsmbsload.h:75)
==2503==    by 0x48C8C26: __wcsmbs_clone_conv (wcsmbsload.c:222)
==2503==    by 0x4890B50: _IO_fwide (iofwide.c:79)
==2503==    by 0x488E359: __wuflow (wgenops.c:225)
==2503==    by 0x488C447: getwc (getwc.c:39)
==2503==    by 0x402581: do_test (tst-wfile-fclose-exit.c:45)
==2503==    by 0x403044: support_test_main (support_test_main.c:350)
==2503==    by 0x4023C4: main (test-driver.c:168)
==2503== 
==2503== 208 bytes in 1 blocks are still reachable in loss record 2 of 2
==2503==    at 0x480B80B: malloc (vg_replace_malloc.c:309)
==2503==    by 0x48477AE: __gconv_lookup_cache (gconv_cache.c:366)
==2503==    by 0x483F9A1: __gconv_find_transform (gconv_db.c:733)
==2503==    by 0x48C8847: __wcsmbs_getfct (wcsmbsload.c:92)
==2503==    by 0x48C89C6: __wcsmbs_load_conv (wcsmbsload.c:189)
==2503==    by 0x48C8C26: get_gconv_fcts (wcsmbsload.h:75)
==2503==    by 0x48C8C26: __wcsmbs_clone_conv (wcsmbsload.c:222)
==2503==    by 0x4890B50: _IO_fwide (iofwide.c:79)
==2503==    by 0x488E359: __wuflow (wgenops.c:225)
==2503==    by 0x488C447: getwc (getwc.c:39)
==2503==    by 0x402581: do_test (tst-wfile-fclose-exit.c:45)
==2503==    by 0x403044: support_test_main (support_test_main.c:350)
==2503==    by 0x4023C4: main (test-driver.c:168)

Note that this leak is only visibile with --leak-check=full
--show-reachable, and only affects programs which use wide streams in
certain ways.

So we may have to tweak the code that frees the gconv cache.  This is
quite risky because at that point, the steps array is still referenced
from the current locale.  For glibc 2.31, we should perhaps switch to
the C locale at the start of __libc_free as a fix.  (In glibc 2.29 and
earlier, I believe we free the gconv cache even though its entries are
still in use.)

Florian

gconv: Check reference count in __gconv_release_cache  [BZ #24677]

This fixes a regression introduced in commit
7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
memory leak [BZ #24583]").

__gconv_release_cache is only ever called with heap-allocated
arrays which contain at least one member.  The statically allocated
ASCII steps are filtered out by __wcsmbs_close_conv.

2019-07-25  Florian Weimer  <fweimer@redhat.com>

	[BZ #24677]
	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
	counter before freeing array.

diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
index 9a456bf825..4db7287cee 100644
--- a/iconv/gconv_cache.c
+++ b/iconv/gconv_cache.c
@@ -446,9 +446,12 @@ __gconv_lookup_cache (const char *toset, const char *fromset,
 void
 __gconv_release_cache (struct __gconv_step *steps, size_t nsteps)
 {
-  if (gconv_cache != NULL)
-    /* The only thing we have to deallocate is the record with the
-       steps.  */
+  /* The only thing we have to deallocate is the record with the
+     steps.  But do not do this if the reference counter is still
+     positive.  This can happen if the steps array was cloned by
+     __wcsmbs_clone_conv.  (The array elements have separate __counter
+     fields, but they are only out of sync temporarily.)  */
+  if (gconv_cache != NULL && steps->__counter == 0)
     free (steps);
 }
 

Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Carlos O'Donell]

On 7/25/19 5:31 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 7/18/19 12:21 PM, Florian Weimer wrote:
>>> This fixes a regression introduced in commit
>>> 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
>>> memory leak [BZ #24583]").
>>>
>>> __gconv_release_cache is only ever called with heap-allocated
>>> arrays which contain at least one member.  The statically allocated
>>> ASCII steps are filtered out by __wcsmbs_close_conv.
>>>
>>> [This variant cleans up __gconv_release_cache somewhat and includes the
>>> test modification suggested by Yann.
>>>
>>> The test does not work due to bug 24824.  I think installing all locales
>>> unconditionally is too costly timewise.  Suggestions how to proceed are
>>> very welcome.]
>>>
>>> 2019-07-18  Florian Weimer  <fweimer@redhat.com>
>>>
>>> 	[BZ #24677]
>>> 	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
>>> 	counter before freeing array.
>>> 	* libio/Makefile (tests-container): Add tst-wfile-fclose-exit.
>>> 	* libio/tst-wfile-fclose-exit.c: New file.
>>> 	* libio/tst-wfile-fclose-exit.root/postclean.req: Likewise.
>>
>> Please post a v2 without a test case.
> 
> Thanks.  Please see below.
> 
>> You'll have to confirm this works by hand and I'm OK with that for
>> now.
> 
> valgrind is clean, but __libc_freeres is not fully effective:

I expected something like this would happen.

> ==2503== 208 bytes in 1 blocks are still reachable in loss record 1 of 2
> ==2503==    at 0x480B80B: malloc (vg_replace_malloc.c:309)
> ==2503==    by 0x4847876: __gconv_lookup_cache (gconv_cache.c:366)
> ==2503==    by 0x483F9A1: __gconv_find_transform (gconv_db.c:733)
> ==2503==    by 0x48C8847: __wcsmbs_getfct (wcsmbsload.c:92)
> ==2503==    by 0x48C89A9: __wcsmbs_load_conv (wcsmbsload.c:186)
> ==2503==    by 0x48C8C26: get_gconv_fcts (wcsmbsload.h:75)
> ==2503==    by 0x48C8C26: __wcsmbs_clone_conv (wcsmbsload.c:222)
> ==2503==    by 0x4890B50: _IO_fwide (iofwide.c:79)
> ==2503==    by 0x488E359: __wuflow (wgenops.c:225)
> ==2503==    by 0x488C447: getwc (getwc.c:39)
> ==2503==    by 0x402581: do_test (tst-wfile-fclose-exit.c:45)
> ==2503==    by 0x403044: support_test_main (support_test_main.c:350)
> ==2503==    by 0x4023C4: main (test-driver.c:168)
> ==2503==
> ==2503== 208 bytes in 1 blocks are still reachable in loss record 2 of 2
> ==2503==    at 0x480B80B: malloc (vg_replace_malloc.c:309)
> ==2503==    by 0x48477AE: __gconv_lookup_cache (gconv_cache.c:366)
> ==2503==    by 0x483F9A1: __gconv_find_transform (gconv_db.c:733)
> ==2503==    by 0x48C8847: __wcsmbs_getfct (wcsmbsload.c:92)
> ==2503==    by 0x48C89C6: __wcsmbs_load_conv (wcsmbsload.c:189)
> ==2503==    by 0x48C8C26: get_gconv_fcts (wcsmbsload.h:75)
> ==2503==    by 0x48C8C26: __wcsmbs_clone_conv (wcsmbsload.c:222)
> ==2503==    by 0x4890B50: _IO_fwide (iofwide.c:79)
> ==2503==    by 0x488E359: __wuflow (wgenops.c:225)
> ==2503==    by 0x488C447: getwc (getwc.c:39)
> ==2503==    by 0x402581: do_test (tst-wfile-fclose-exit.c:45)
> ==2503==    by 0x403044: support_test_main (support_test_main.c:350)
> ==2503==    by 0x4023C4: main (test-driver.c:168)
> 
> Note that this leak is only visibile with --leak-check=full
> --show-reachable, and only affects programs which use wide streams in
> certain ways.

That's fine. Thanks for checking. It's much better than double-free.

> So we may have to tweak the code that frees the gconv cache.  This is
> quite risky because at that point, the steps array is still referenced
> from the current locale.  For glibc 2.31, we should perhaps switch to
> the C locale at the start of __libc_free as a fix.  (In glibc 2.29 and
> earlier, I believe we free the gconv cache even though its entries are
> still in use.)

Yes, I assume you mean __libc_freeres? Yes, in __libc_freeres we can do
all sorts of things like switch to the C locale, and then free all the
objects.

  
> Florian
> 
> gconv: Check reference count in __gconv_release_cache  [BZ #24677]
> 
> This fixes a regression introduced in commit
> 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related
> memory leak [BZ #24583]").
> 
> __gconv_release_cache is only ever called with heap-allocated
> arrays which contain at least one member.  The statically allocated
> ASCII steps are filtered out by __wcsmbs_close_conv.
> 
> 2019-07-25  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24677]
> 	* iconv/gconv_cache.c (__gconv_release_cache): Check reference
> 	counter before freeing array.
> 
> diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
> index 9a456bf825..4db7287cee 100644
> --- a/iconv/gconv_cache.c
> +++ b/iconv/gconv_cache.c
> @@ -446,9 +446,12 @@ __gconv_lookup_cache (const char *toset, const char *fromset,
>   void
>   __gconv_release_cache (struct __gconv_step *steps, size_t nsteps)
>   {
> -  if (gconv_cache != NULL)
> -    /* The only thing we have to deallocate is the record with the
> -       steps.  */
> +  /* The only thing we have to deallocate is the record with the
> +     steps.  But do not do this if the reference counter is still
> +     positive.  This can happen if the steps array was cloned by
> +     __wcsmbs_clone_conv.  (The array elements have separate __counter
> +     fields, but they are only out of sync temporarily.)  */
> +  if (gconv_cache != NULL && steps->__counter == 0)
>       free (steps);
>   }
>   
> 

OK. Please commit to master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

-- 
Cheers,
Carlos.