libio: Fix gconv-related memory leak [BZ #24583]

Message ID 87sgt9jp5x.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • libio: Fix gconv-related memory leak [BZ #24583]
Related show

Commit Message

Florian Weimer May 20, 2019, 12:17 p.m.
__gconv_close_transform performs locking internally, so remove
the locking operations.

2019-05-20  Florian Weimer  <fweimer@redhat.com>

	* libio/iofclose.c (_IO_new_fclose): Call __gconv_close_transform
	instead of __gconv_release_step.
	* libio/Makefile (tests): Add tst-wfile-gconv.
	(tst-wfile-gconv-ENV): Enable mtrace.
	(generated): Add tst-wfile-gconv.mtrace, tst-wfile-gconv.check.
	(tests-special): Add tst-wfile-gconv-mem.out.
	(tst-wfile-gconv.out): Depend on locales.
	(tst-wfile-gconv-mem.out): Add mtrace rule.
	* libio/tst-wfile-gconv.c: New file.

Comments

Andreas Schwab May 20, 2019, 2:05 p.m. | #1
On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:

> diff --git a/libio/iofclose.c b/libio/iofclose.c

> index 8a80dd0b78..f4c7299db3 100644

> --- a/libio/iofclose.c

> +++ b/libio/iofclose.c

> @@ -61,10 +61,10 @@ _IO_new_fclose (FILE *fp)

>  	 the conversion functions.  */

>        struct _IO_codecvt *cc = fp->_codecvt;

>  

> -      __libc_lock_lock (__gconv_lock);

> -      __gconv_release_step (cc->__cd_in.__cd.__steps);

> -      __gconv_release_step (cc->__cd_out.__cd.__steps);

> -      __libc_lock_unlock (__gconv_lock);

> +      __gconv_close_transform (cc->__cd_in.__cd.__steps,

> +			       cc->__cd_in.__cd.__nsteps);

> +      __gconv_close_transform (cc->__cd_out.__cd.__steps,

> +			       cc->__cd_out.__cd.__nsteps);


Are the __steps always allocated?  In get_gconv_fcts I see them being
set to static data.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer May 20, 2019, 3:40 p.m. | #2
* Andreas Schwab:

> On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> diff --git a/libio/iofclose.c b/libio/iofclose.c

>> index 8a80dd0b78..f4c7299db3 100644

>> --- a/libio/iofclose.c

>> +++ b/libio/iofclose.c

>> @@ -61,10 +61,10 @@ _IO_new_fclose (FILE *fp)

>>  	 the conversion functions.  */

>>        struct _IO_codecvt *cc = fp->_codecvt;

>>  

>> -      __libc_lock_lock (__gconv_lock);

>> -      __gconv_release_step (cc->__cd_in.__cd.__steps);

>> -      __gconv_release_step (cc->__cd_out.__cd.__steps);

>> -      __libc_lock_unlock (__gconv_lock);

>> +      __gconv_close_transform (cc->__cd_in.__cd.__steps,

>> +			       cc->__cd_in.__cd.__nsteps);

>> +      __gconv_close_transform (cc->__cd_out.__cd.__steps,

>> +			       cc->__cd_out.__cd.__nsteps);

>

> Are the __steps always allocated?  In get_gconv_fcts I see them being

> set to static data.


Correct, this needs to be handled differently.  It is difficult to
reproduce because the test framework disables the gconv cache (even when
running in a container).

I will try to fix the test framework first.

Thanks,
Florian
Florian Weimer May 20, 2019, 7:04 p.m. | #3
* Florian Weimer:

> * Andreas Schwab:

>

>> On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:

>>

>>> diff --git a/libio/iofclose.c b/libio/iofclose.c

>>> index 8a80dd0b78..f4c7299db3 100644

>>> --- a/libio/iofclose.c

>>> +++ b/libio/iofclose.c

>>> @@ -61,10 +61,10 @@ _IO_new_fclose (FILE *fp)

>>>  	 the conversion functions.  */

>>>        struct _IO_codecvt *cc = fp->_codecvt;

>>>  

>>> -      __libc_lock_lock (__gconv_lock);

>>> -      __gconv_release_step (cc->__cd_in.__cd.__steps);

>>> -      __gconv_release_step (cc->__cd_out.__cd.__steps);

>>> -      __libc_lock_unlock (__gconv_lock);

>>> +      __gconv_close_transform (cc->__cd_in.__cd.__steps,

>>> +			       cc->__cd_in.__cd.__nsteps);

>>> +      __gconv_close_transform (cc->__cd_out.__cd.__steps,

>>> +			       cc->__cd_out.__cd.__nsteps);

>>

>> Are the __steps always allocated?  In get_gconv_fcts I see them being

>> set to static data.

>

> Correct, this needs to be handled differently.  It is difficult to

> reproduce because the test framework disables the gconv cache (even when

> running in a container).

>

> I will try to fix the test framework first.


I've already posted the sbindir patch and the test-in-container fix.

The patch below should fix the memory leak, while avoiding the static
allocation issue you've identified.  Ideally, we would use struct
gconv_fcts in libio as well, but I think that's best handled as a
separate cleanup, after the codecvt vtable removal.

Thanks,
Florian

libio: Fix gconv-related memory leak [BZ #24583]

struct gconv_fcts for the C locale is statically allocated,
and __gconv_close_transform deallocates the steps object.
Therefore this commit introduces __wcsmbs_close_conv to avoid
freeing the statically allocated step objects.

2019-05-20  Florian Weimer  <fweimer@redhat.com>

	[BZ #24583]
	* wcsmbs/wcsmbsload.h (__wcsmbs_close_conv): Declare.
	* wcsmbs/wcsmbsload.c (__wcsmbs_close_conv): Define.
	* libio/iofclose.c (_IO_new_fclose): Call __wcsmbs_close_conv
	instead of __gconv_release_step.
	* libio/Makefile (tests): Add tst-wfile-gconv.
	(tests-container): Add tst-wfile-ascii.
	(tst-wfile-gconv-ENV): Enable mtrace.
	(generated): Add tst-wfile-gconv.mtrace, tst-wfile-gconv.check.
	(tests-special): Add tst-wfile-gconv-mem.out.
	(tst-wfile-gconv.out): Depend on locales.
	(tst-wfile-gconv-mem.out): Add mtrace rule.
	* libio/tst-wfile-ascii.c: New file.
	* libio/tst-wfile-gconv.c: Likewise.

diff --git a/libio/Makefile b/libio/Makefile
index a5236c7042..7d53cb06b0 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -66,7 +66,11 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
 	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
 	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
-	tst-wfile-sync
+	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
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -163,10 +167,12 @@ tst_wprintf2-ARGS = "Some Text"
 test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace
 tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace
 tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace
+tst-wfile-gconv-ENV = MALLOC_TRACE=$(objpfx)tst-wfile-gconv.mtrace
 
 generated += test-fmemopen.mtrace test-fmemopen.check
 generated += tst-fopenloc.mtrace tst-fopenloc.check
 generated += tst-bz22415.mtrace tst-bz22415.check
+generated += tst-wfile-gconv.mtrace tst-wfile-gconv.check
 
 aux	:= fileops genops stdfiles stdio strops
 
@@ -181,7 +187,8 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops	\
 
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \
-		 $(objpfx)tst-bz22415-mem.out
+		 $(objpfx)tst-bz22415-mem.out \
+		 $(objpfx)tst-wfile-gconv-mem.out
 ifeq (yes,$(build-shared))
 # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared
 # library is enabled since they depend on tst-fopenloc.out.
@@ -214,6 +221,7 @@ $(objpfx)tst-ungetwc2.out: $(gen-locales)
 $(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)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
@@ -237,3 +245,7 @@ $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out
 $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out
 	$(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \
 	$(evaluate-test)
+
+$(objpfx)tst-wfile-gconv-mem.out: $(objpfx)tst-wfile-gconv.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-wfile-gconv.mtrace > $@; \
+	$(evaluate-test)
diff --git a/libio/iofclose.c b/libio/iofclose.c
index 8a80dd0b78..c03c6cf57c 100644
--- a/libio/iofclose.c
+++ b/libio/iofclose.c
@@ -26,8 +26,8 @@
 
 #include "libioP.h"
 #include <stdlib.h>
-#include "../iconv/gconv_int.h"
 #include <shlib-compat.h>
+#include <wcsmbs/wcsmbsload.h>
 
 int
 _IO_new_fclose (FILE *fp)
@@ -60,11 +60,14 @@ _IO_new_fclose (FILE *fp)
       /* This stream has a wide orientation.  This means we have to free
 	 the conversion functions.  */
       struct _IO_codecvt *cc = fp->_codecvt;
-
-      __libc_lock_lock (__gconv_lock);
-      __gconv_release_step (cc->__cd_in.__cd.__steps);
-      __gconv_release_step (cc->__cd_out.__cd.__steps);
-      __libc_lock_unlock (__gconv_lock);
+      struct gconv_fcts conv =
+	{
+	 .towc = cc->__cd_in.__cd.__steps,
+	 .towc_nsteps = cc->__cd_in.__cd.__nsteps,
+	 .tomb = cc->__cd_out.__cd.__steps,
+	 .tomb_nsteps = cc->__cd_out.__cd.__nsteps,
+	};
+      __wcsmbs_close_conv (&conv);
     }
   else
     {
diff --git a/libio/tst-wfile-ascii.c b/libio/tst-wfile-ascii.c
new file mode 100644
index 0000000000..77272fc125
--- /dev/null
+++ b/libio/tst-wfile-ascii.c
@@ -0,0 +1,51 @@
+/* Test ASCII gconv module followed by cache initialization.
+   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 <stdlib.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xstdio.h>
+#include <wchar.h>
+
+static int
+do_test (void)
+{
+  /* 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);
+  }
+
+  /* Use built-in ASCII gconv module, without triggering cache
+     initialization.  */
+  FILE *fp1 = xfopen ("/dev/zero", "r");
+  TEST_COMPARE (fwide (fp1, 1), 1);
+
+  /* Use non-ASCII gconv module and trigger gconv cache
+     initialization.  */
+  FILE *fp2 = xfopen ("/dev/zero", "r,ccs=UTF-8");
+  TEST_COMPARE (fwide (fp2, 0), 1);
+
+  xfclose (fp1);
+  xfclose (fp2);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/libio/tst-wfile-gconv.c b/libio/tst-wfile-gconv.c
new file mode 100644
index 0000000000..de603b32d2
--- /dev/null
+++ b/libio/tst-wfile-gconv.c
@@ -0,0 +1,36 @@
+/* Test that non-built-in gconv modules do not cause memory leak (bug 24583).
+   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 <mcheck.h>
+#include <support/check.h>
+#include <support/xstdio.h>
+
+static int
+do_test (void)
+{
+  mtrace ();
+
+  TEST_VERIFY_EXIT (setlocale (LC_ALL, "ja_JP.EUC-JP") != NULL);
+  xfclose (xfopen ("/etc/passwd", "r,ccs=UTF-8"));
+  xfclose (xfopen ("/etc/passwd", "r"));
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c
index 5494d0a23e..cff7ca85f9 100644
--- a/wcsmbs/wcsmbsload.c
+++ b/wcsmbs/wcsmbsload.c
@@ -265,3 +265,17 @@ _nl_cleanup_ctype (struct __locale_data *locale)
       free ((char *) data);
     }
 }
+
+/* Free the specified conversion functions (but not CONV itself).  */
+void
+__wcsmbs_close_conv (struct gconv_fcts *conv)
+{
+  /* Nothing to do if both conversions are statically allocated.  */
+  if (conv->towc == &to_wc && conv->tomb == &to_mb)
+    return;
+
+  if (conv->towc != &to_wc)
+      __gconv_close_transform (conv->towc, conv->towc_nsteps);
+  if (conv->tomb != &to_mb)
+      __gconv_close_transform (conv->tomb, conv->tomb_nsteps);
+}
diff --git a/wcsmbs/wcsmbsload.h b/wcsmbs/wcsmbsload.h
index 6ccad4b3ba..c2fffbd914 100644
--- a/wcsmbs/wcsmbsload.h
+++ b/wcsmbs/wcsmbsload.h
@@ -51,6 +51,7 @@ extern int __wcsmbs_named_conv (struct gconv_fcts *copy, const char *name)
 /* Function used for the `private.cleanup' hook.  */
 extern void _nl_cleanup_ctype (struct __locale_data *) attribute_hidden;
 
+extern void __wcsmbs_close_conv (struct gconv_fcts *conv) attribute_hidden;
 
 #include <iconv/gconv_int.h>
Andreas Schwab May 21, 2019, 7:19 a.m. | #4
On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:

> diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c

> index 5494d0a23e..cff7ca85f9 100644

> --- a/wcsmbs/wcsmbsload.c

> +++ b/wcsmbs/wcsmbsload.c

> @@ -265,3 +265,17 @@ _nl_cleanup_ctype (struct __locale_data *locale)

>        free ((char *) data);

>      }

>  }

> +

> +/* Free the specified conversion functions (but not CONV itself).  */

> +void

> +__wcsmbs_close_conv (struct gconv_fcts *conv)

> +{

> +  /* Nothing to do if both conversions are statically allocated.  */

> +  if (conv->towc == &to_wc && conv->tomb == &to_mb)

> +    return;


That looks redundant.

> +

> +  if (conv->towc != &to_wc)

> +      __gconv_close_transform (conv->towc, conv->towc_nsteps);

> +  if (conv->tomb != &to_mb)

> +      __gconv_close_transform (conv->tomb, conv->tomb_nsteps);

> +}


Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer May 21, 2019, 8:53 a.m. | #5
* Andreas Schwab:

> On Mai 20 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c

>> index 5494d0a23e..cff7ca85f9 100644

>> --- a/wcsmbs/wcsmbsload.c

>> +++ b/wcsmbs/wcsmbsload.c

>> @@ -265,3 +265,17 @@ _nl_cleanup_ctype (struct __locale_data *locale)

>>        free ((char *) data);

>>      }

>>  }

>> +

>> +/* Free the specified conversion functions (but not CONV itself).  */

>> +void

>> +__wcsmbs_close_conv (struct gconv_fcts *conv)

>> +{

>> +  /* Nothing to do if both conversions are statically allocated.  */

>> +  if (conv->towc == &to_wc && conv->tomb == &to_mb)

>> +    return;

>

> That looks redundant.


Yes, it was a leftover from when the locking was different.  New patch
below.

The patch also tweaks the test so that it works around the tained
environment provided by the test-in-container framework, by unsetting
two environment variables.

Thanks,
Florian

libio: Fix gconv-related memory leak [BZ #24583]

struct gconv_fcts for the C locale is statically allocated,
and __gconv_close_transform deallocates the steps object.
Therefore this commit introduces __wcsmbs_close_conv to avoid
freeing the statically allocated steps objects.

2019-05-21  Florian Weimer  <fweimer@redhat.com>

	[BZ #24583]
	* wcsmbs/wcsmbsload.h (__wcsmbs_close_conv): Declare.
	* wcsmbs/wcsmbsload.c (__wcsmbs_close_conv): Define.
	* libio/iofclose.c (_IO_new_fclose): Call __wcsmbs_close_conv
	instead of __gconv_release_step.
	* libio/Makefile (tests): Add tst-wfile-gconv.
	(tests-container): Add tst-wfile-ascii.
	(tst-wfile-gconv-ENV): Enable mtrace.
	(generated): Add tst-wfile-gconv.mtrace, tst-wfile-gconv.check.
	(tests-special): Add tst-wfile-gconv-mem.out.
	(tst-wfile-gconv.out): Depend on locales.
	(tst-wfile-gconv-mem.out): Add mtrace rule.
	* libio/tst-wfile-ascii.c: New file.
	* libio/tst-wfile-gconv.c: Likewise.

diff --git a/libio/Makefile b/libio/Makefile
index a5236c7042..7d53cb06b0 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -66,7 +66,11 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
 	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
 	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
-	tst-wfile-sync
+	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
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -163,10 +167,12 @@ tst_wprintf2-ARGS = "Some Text"
 test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace
 tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace
 tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace
+tst-wfile-gconv-ENV = MALLOC_TRACE=$(objpfx)tst-wfile-gconv.mtrace
 
 generated += test-fmemopen.mtrace test-fmemopen.check
 generated += tst-fopenloc.mtrace tst-fopenloc.check
 generated += tst-bz22415.mtrace tst-bz22415.check
+generated += tst-wfile-gconv.mtrace tst-wfile-gconv.check
 
 aux	:= fileops genops stdfiles stdio strops
 
@@ -181,7 +187,8 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops	\
 
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \
-		 $(objpfx)tst-bz22415-mem.out
+		 $(objpfx)tst-bz22415-mem.out \
+		 $(objpfx)tst-wfile-gconv-mem.out
 ifeq (yes,$(build-shared))
 # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared
 # library is enabled since they depend on tst-fopenloc.out.
@@ -214,6 +221,7 @@ $(objpfx)tst-ungetwc2.out: $(gen-locales)
 $(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)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
@@ -237,3 +245,7 @@ $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out
 $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out
 	$(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \
 	$(evaluate-test)
+
+$(objpfx)tst-wfile-gconv-mem.out: $(objpfx)tst-wfile-gconv.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-wfile-gconv.mtrace > $@; \
+	$(evaluate-test)
diff --git a/libio/iofclose.c b/libio/iofclose.c
index 8a80dd0b78..c03c6cf57c 100644
--- a/libio/iofclose.c
+++ b/libio/iofclose.c
@@ -26,8 +26,8 @@
 
 #include "libioP.h"
 #include <stdlib.h>
-#include "../iconv/gconv_int.h"
 #include <shlib-compat.h>
+#include <wcsmbs/wcsmbsload.h>
 
 int
 _IO_new_fclose (FILE *fp)
@@ -60,11 +60,14 @@ _IO_new_fclose (FILE *fp)
       /* This stream has a wide orientation.  This means we have to free
 	 the conversion functions.  */
       struct _IO_codecvt *cc = fp->_codecvt;
-
-      __libc_lock_lock (__gconv_lock);
-      __gconv_release_step (cc->__cd_in.__cd.__steps);
-      __gconv_release_step (cc->__cd_out.__cd.__steps);
-      __libc_lock_unlock (__gconv_lock);
+      struct gconv_fcts conv =
+	{
+	 .towc = cc->__cd_in.__cd.__steps,
+	 .towc_nsteps = cc->__cd_in.__cd.__nsteps,
+	 .tomb = cc->__cd_out.__cd.__steps,
+	 .tomb_nsteps = cc->__cd_out.__cd.__nsteps,
+	};
+      __wcsmbs_close_conv (&conv);
     }
   else
     {
diff --git a/libio/tst-wfile-ascii.c b/libio/tst-wfile-ascii.c
new file mode 100644
index 0000000000..7514289a7b
--- /dev/null
+++ b/libio/tst-wfile-ascii.c
@@ -0,0 +1,56 @@
+/* Test ASCII gconv module followed by cache initialization.
+   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 <stdlib.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xstdio.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);
+  }
+
+  /* Use built-in ASCII gconv module, without triggering cache
+     initialization.  */
+  FILE *fp1 = xfopen ("/dev/zero", "r");
+  TEST_COMPARE (fwide (fp1, 1), 1);
+
+  /* Use non-ASCII gconv module and trigger gconv cache
+     initialization.  */
+  FILE *fp2 = xfopen ("/dev/zero", "r,ccs=UTF-8");
+  TEST_COMPARE (fwide (fp2, 0), 1);
+
+  xfclose (fp1);
+  xfclose (fp2);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/libio/tst-wfile-gconv.c b/libio/tst-wfile-gconv.c
new file mode 100644
index 0000000000..de603b32d2
--- /dev/null
+++ b/libio/tst-wfile-gconv.c
@@ -0,0 +1,36 @@
+/* Test that non-built-in gconv modules do not cause memory leak (bug 24583).
+   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 <mcheck.h>
+#include <support/check.h>
+#include <support/xstdio.h>
+
+static int
+do_test (void)
+{
+  mtrace ();
+
+  TEST_VERIFY_EXIT (setlocale (LC_ALL, "ja_JP.EUC-JP") != NULL);
+  xfclose (xfopen ("/etc/passwd", "r,ccs=UTF-8"));
+  xfclose (xfopen ("/etc/passwd", "r"));
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c
index 5494d0a23e..10e1a4f4f5 100644
--- a/wcsmbs/wcsmbsload.c
+++ b/wcsmbs/wcsmbsload.c
@@ -265,3 +265,13 @@ _nl_cleanup_ctype (struct __locale_data *locale)
       free ((char *) data);
     }
 }
+
+/* Free the specified conversion functions (but not CONV itself).  */
+void
+__wcsmbs_close_conv (struct gconv_fcts *conv)
+{
+  if (conv->towc != &to_wc)
+      __gconv_close_transform (conv->towc, conv->towc_nsteps);
+  if (conv->tomb != &to_mb)
+      __gconv_close_transform (conv->tomb, conv->tomb_nsteps);
+}
diff --git a/wcsmbs/wcsmbsload.h b/wcsmbs/wcsmbsload.h
index 6ccad4b3ba..c2fffbd914 100644
--- a/wcsmbs/wcsmbsload.h
+++ b/wcsmbs/wcsmbsload.h
@@ -51,6 +51,7 @@ extern int __wcsmbs_named_conv (struct gconv_fcts *copy, const char *name)
 /* Function used for the `private.cleanup' hook.  */
 extern void _nl_cleanup_ctype (struct __locale_data *) attribute_hidden;
 
+extern void __wcsmbs_close_conv (struct gconv_fcts *conv) attribute_hidden;
 
 #include <iconv/gconv_int.h>
Andreas Schwab May 21, 2019, 9:22 a.m. | #6
On Mai 21 2019, Florian Weimer <fweimer@redhat.com> wrote:

> 	[BZ #24583]

> 	* wcsmbs/wcsmbsload.h (__wcsmbs_close_conv): Declare.

> 	* wcsmbs/wcsmbsload.c (__wcsmbs_close_conv): Define.

> 	* libio/iofclose.c (_IO_new_fclose): Call __wcsmbs_close_conv

> 	instead of __gconv_release_step.

> 	* libio/Makefile (tests): Add tst-wfile-gconv.

> 	(tests-container): Add tst-wfile-ascii.

> 	(tst-wfile-gconv-ENV): Enable mtrace.

> 	(generated): Add tst-wfile-gconv.mtrace, tst-wfile-gconv.check.

> 	(tests-special): Add tst-wfile-gconv-mem.out.

> 	(tst-wfile-gconv.out): Depend on locales.

> 	(tst-wfile-gconv-mem.out): Add mtrace rule.

> 	* libio/tst-wfile-ascii.c: New file.

> 	* libio/tst-wfile-gconv.c: Likewise.


Ok.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Patch

diff --git a/libio/Makefile b/libio/Makefile
index a5236c7042..641f4f00af 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -66,7 +66,7 @@  tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
 	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
 	tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
-	tst-wfile-sync
+	tst-wfile-sync tst-wfile-gconv
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -163,10 +163,12 @@  tst_wprintf2-ARGS = "Some Text"
 test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace
 tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace
 tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace
+tst-wfile-gconv-ENV = MALLOC_TRACE=$(objpfx)tst-wfile-gconv.mtrace
 
 generated += test-fmemopen.mtrace test-fmemopen.check
 generated += tst-fopenloc.mtrace tst-fopenloc.check
 generated += tst-bz22415.mtrace tst-bz22415.check
+generated += tst-wfile-gconv.mtrace tst-wfile-gconv.check
 
 aux	:= fileops genops stdfiles stdio strops
 
@@ -181,7 +183,7 @@  shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops	\
 
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \
-		 $(objpfx)tst-bz22415-mem.out
+		 $(objpfx)tst-bz22415-mem.out $(objpfx)tst-wfile-gconv-mem.out
 ifeq (yes,$(build-shared))
 # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared
 # library is enabled since they depend on tst-fopenloc.out.
@@ -214,6 +216,7 @@  $(objpfx)tst-ungetwc2.out: $(gen-locales)
 $(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)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
@@ -237,3 +240,7 @@  $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out
 $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out
 	$(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \
 	$(evaluate-test)
+
+$(objpfx)tst-wfile-gconv-mem.out: $(objpfx)tst-wfile-gconv.out
+	$(common-objpfx)malloc/mtrace $(objpfx)tst-wfile-gconv.mtrace > $@; \
+	$(evaluate-test)
diff --git a/libio/iofclose.c b/libio/iofclose.c
index 8a80dd0b78..f4c7299db3 100644
--- a/libio/iofclose.c
+++ b/libio/iofclose.c
@@ -61,10 +61,10 @@  _IO_new_fclose (FILE *fp)
 	 the conversion functions.  */
       struct _IO_codecvt *cc = fp->_codecvt;
 
-      __libc_lock_lock (__gconv_lock);
-      __gconv_release_step (cc->__cd_in.__cd.__steps);
-      __gconv_release_step (cc->__cd_out.__cd.__steps);
-      __libc_lock_unlock (__gconv_lock);
+      __gconv_close_transform (cc->__cd_in.__cd.__steps,
+			       cc->__cd_in.__cd.__nsteps);
+      __gconv_close_transform (cc->__cd_out.__cd.__steps,
+			       cc->__cd_out.__cd.__nsteps);
     }
   else
     {
diff --git a/libio/tst-wfile-gconv.c b/libio/tst-wfile-gconv.c
new file mode 100644
index 0000000000..de603b32d2
--- /dev/null
+++ b/libio/tst-wfile-gconv.c
@@ -0,0 +1,36 @@ 
+/* Test that non-built-in gconv modules do not cause memory leak (bug 24583).
+   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 <mcheck.h>
+#include <support/check.h>
+#include <support/xstdio.h>
+
+static int
+do_test (void)
+{
+  mtrace ();
+
+  TEST_VERIFY_EXIT (setlocale (LC_ALL, "ja_JP.EUC-JP") != NULL);
+  xfclose (xfopen ("/etc/passwd", "r,ccs=UTF-8"));
+  xfclose (xfopen ("/etc/passwd", "r"));
+
+  return 0;
+}
+
+#include <support/test-driver.c>