* [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).