* [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
@ 2019-05-20 12:17 Florian Weimer
2019-05-20 14:05 ` Andreas Schwab
0 siblings, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2019-05-20 12:17 UTC (permalink / raw)
To: libc-alpha
__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.
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>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
2019-05-20 12:17 [PATCH] libio: Fix gconv-related memory leak [BZ #24583] Florian Weimer
@ 2019-05-20 14:05 ` Andreas Schwab
2019-05-20 15:40 ` Florian Weimer
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Schwab @ 2019-05-20 14:05 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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."
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
2019-05-20 14:05 ` Andreas Schwab
@ 2019-05-20 15:40 ` Florian Weimer
2019-05-20 19:04 ` Florian Weimer
0 siblings, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2019-05-20 15:40 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
2019-05-20 15:40 ` Florian Weimer
@ 2019-05-20 19:04 ` Florian Weimer
2019-05-21 7:19 ` Andreas Schwab
0 siblings, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2019-05-20 19:04 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* 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>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
2019-05-20 19:04 ` Florian Weimer
@ 2019-05-21 7:19 ` Andreas Schwab
2019-05-21 8:53 ` Florian Weimer
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Schwab @ 2019-05-21 7:19 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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."
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
2019-05-21 7:19 ` Andreas Schwab
@ 2019-05-21 8:53 ` Florian Weimer
2019-05-21 9:22 ` Andreas Schwab
0 siblings, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2019-05-21 8:53 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
* 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>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libio: Fix gconv-related memory leak [BZ #24583]
2019-05-21 8:53 ` Florian Weimer
@ 2019-05-21 9:22 ` Andreas Schwab
0 siblings, 0 replies; 7+ messages in thread
From: Andreas Schwab @ 2019-05-21 9:22 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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."
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-21 9:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-20 12:17 [PATCH] libio: Fix gconv-related memory leak [BZ #24583] Florian Weimer
2019-05-20 14:05 ` Andreas Schwab
2019-05-20 15:40 ` Florian Weimer
2019-05-20 19:04 ` Florian Weimer
2019-05-21 7:19 ` Andreas Schwab
2019-05-21 8:53 ` Florian Weimer
2019-05-21 9:22 ` Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).