| Message ID | 87zhkur0rn.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iconv: Revert steps array reference counting changes | expand |
On 7/31/19 5:50 AM, Florian Weimer wrote: > The changes introduce a memory leak for gconv steps arrays whose > first element is an internal conversion, which has a fixed > reference count which is not decremented. As a result, after the > change in commit 50ce3eae5ba304650459d4441d7d246a7cefc26f, the steps > array is never freed, resulting in an unbounded memory leak. If you have done the analysis and the result is indeed an unbounded memory leak then yes, we should revert this back to the original state. I'm curious how it happens, but we can talk about that in the coming weeks. There must be something about the wide usage that I don't follow. This looks good to me and I verified it reverts the fix for BZ #24677 and BZ #24583 (the real fix) until we can review again how to cleanup the steps array in a way which doesn't cause a leak for each file descriptor that has a non-zero steps counter. OK for master. Please push and then I'm close to cutting 2.30. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > This reverts commit 50ce3eae5ba304650459d4441d7d246a7cefc26f > ("gconv: Check reference count in __gconv_release_cache > [BZ #24677]") and commit 7e740ab2e7be7d83b75513aa406e0b10875f7f9c > ("libio: Fix gconv-related memory leak [BZ #24583]"). It > reintroduces bug 24583. (Bug 24677 was just a regression caused by > the second commit.) > > 2019-07-31 Florian Weimer <fweimer@redhat.com> > > [BZ #24583] > [BZ #24677] > iconv, libio: Revert reference counting changes. > * iconv/gconv_cache.c (__gconv_release_cache): Unconditionally > free the steps array. > * libio/Makefile (tests): Remove tst-wfile-gconv. > (tests-container): Do not add tst-wfile-ascii. > (tst-wfile-gconv-ENV): Do not set. > (generated): Do not add tst-wfile-gconv.mtrace, > tst-wfile-gconv.check. > [($run-built-tests)] (tests-special): Do not add > tst-wfile-gconv-mem.out. > (tst-wfile-gconv.out, tst-wfile-gconv-mem.out): Remove targets. > * libio/iofclose.c (_IO_new_fclose): Call __gconv_release_step > instead of __wcsmbs_clone_conv. > * wcsmbs/wcsmbsload.c (__wcsmbs_clone_conv): Remove definition. > * wcsmbs/wcsmbsload.h (__wcsmbs_clone_conv): Remove declaration. > > diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c > index 4db7287cee..9a456bf825 100644 > --- a/iconv/gconv_cache.c > +++ b/iconv/gconv_cache.c > @@ -446,12 +446,9 @@ __gconv_lookup_cache (const char *toset, const char *fromset, > void > __gconv_release_cache (struct __gconv_step *steps, size_t nsteps) > { > - /* 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) > + if (gconv_cache != NULL) > + /* The only thing we have to deallocate is the record with the > + steps. */ > free (steps); > } > > diff --git a/libio/Makefile b/libio/Makefile > index 6e594b8ec5..4a3637f046 100644 > --- a/libio/Makefile > +++ b/libio/Makefile > @@ -66,11 +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-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 > + tst-wfile-sync > > tests-internal = tst-vtables tst-vtables-interposed tst-readline > > @@ -173,12 +169,10 @@ 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-bz24228-ENV = MALLOC_TRACE=$(objpfx)tst-bz24228.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 > > @@ -194,8 +188,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-wfile-gconv-mem.out > + $(objpfx)tst-bz22415-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. > @@ -229,7 +222,6 @@ $(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 > @@ -257,7 +249,3 @@ $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out > $(objpfx)tst-bz24228-mem.out: $(objpfx)tst-bz24228.out > $(common-objpfx)malloc/mtrace $(objpfx)tst-bz24228.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 c03c6cf57c..8a80dd0b78 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,14 +60,11 @@ _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; > - 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); > + > + __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); > } > else > { > diff --git a/libio/tst-wfile-ascii.c b/libio/tst-wfile-ascii.c > deleted file mode 100644 > index 7514289a7b..0000000000 > --- a/libio/tst-wfile-ascii.c > +++ /dev/null > @@ -1,56 +0,0 @@ > -/* 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 > deleted file mode 100644 > index de603b32d2..0000000000 > --- a/libio/tst-wfile-gconv.c > +++ /dev/null > @@ -1,36 +0,0 @@ > -/* 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 840d4abc44..6648365d82 100644 > --- a/wcsmbs/wcsmbsload.c > +++ b/wcsmbs/wcsmbsload.c > @@ -279,13 +279,3 @@ _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 c2fffbd914..6ccad4b3ba 100644 > --- a/wcsmbs/wcsmbsload.h > +++ b/wcsmbs/wcsmbsload.h > @@ -51,7 +51,6 @@ 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> > >
* Carlos O'Donell: > On 7/31/19 5:50 AM, Florian Weimer wrote: >> The changes introduce a memory leak for gconv steps arrays whose >> first element is an internal conversion, which has a fixed >> reference count which is not decremented. As a result, after the >> change in commit 50ce3eae5ba304650459d4441d7d246a7cefc26f, the steps >> array is never freed, resulting in an unbounded memory leak. > > If you have done the analysis and the result is indeed an unbounded > memory leak then yes, we should revert this back to the original state. > I'm curious how it happens, but we can talk about that in the coming > weeks. There must be something about the wide usage that I don't follow. I think we populate the array with reference count 1, see __gconv_load_cache: result[idx].__counter = 1; result[idx].__data = NULL; if (strtab[extra->module[idx].dir_offset] != '\0') … else /* It's a builtin transformation. */ __gconv_get_builtin_trans (strtab + extra->module[idx].name_offset, &result[idx]); __gconv_get_builtin_trans does not updated __counter. But __gconv_release_step looks does this: void __gconv_release_step (struct __gconv_step *step) { /* Skip builtin modules; they are not reference counted. */ if (step->__shlib_handle != NULL && --step->__counter == 0) So we never lower the reference count to zero. That's also what I saw in the debugger. Since it impacts iconv_open/iconv_close and the allocation is stored only in the iconv handle itself, it's automatically unbounded. I pushed this and updated Bugzilla. Please do another build to check that the tree is fine. It's very late here. Thanks, Florian
On 7/31/19 5:11 PM, Florian Weimer wrote: > * Carlos O'Donell: > >> On 7/31/19 5:50 AM, Florian Weimer wrote: >>> The changes introduce a memory leak for gconv steps arrays whose >>> first element is an internal conversion, which has a fixed >>> reference count which is not decremented. As a result, after the >>> change in commit 50ce3eae5ba304650459d4441d7d246a7cefc26f, the steps >>> array is never freed, resulting in an unbounded memory leak. >> >> If you have done the analysis and the result is indeed an unbounded >> memory leak then yes, we should revert this back to the original state. >> I'm curious how it happens, but we can talk about that in the coming >> weeks. There must be something about the wide usage that I don't follow. > > I think we populate the array with reference count 1, see > __gconv_load_cache: > > result[idx].__counter = 1; > result[idx].__data = NULL; > > if (strtab[extra->module[idx].dir_offset] != '\0') > … > else > /* It's a builtin transformation. */ > __gconv_get_builtin_trans (strtab > + extra->module[idx].name_offset, > &result[idx]); > > __gconv_get_builtin_trans does not updated __counter. > > But __gconv_release_step looks does this: > > void > __gconv_release_step (struct __gconv_step *step) > { > /* Skip builtin modules; they are not reference counted. */ > if (step->__shlib_handle != NULL && --step->__counter == 0) > > So we never lower the reference count to zero. That's also what I saw > in the debugger. Since it impacts iconv_open/iconv_close and the > allocation is stored only in the iconv handle itself, it's automatically > unbounded. > > I pushed this and updated Bugzilla. Please do another build to check > that the tree is fine. It's very late here. I did an x86_64 and i686 build with testing and it was fine.
diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c index 4db7287cee..9a456bf825 100644 --- a/iconv/gconv_cache.c +++ b/iconv/gconv_cache.c @@ -446,12 +446,9 @@ __gconv_lookup_cache (const char *toset, const char *fromset, void __gconv_release_cache (struct __gconv_step *steps, size_t nsteps) { - /* 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) + if (gconv_cache != NULL) + /* The only thing we have to deallocate is the record with the + steps. */ free (steps); } diff --git a/libio/Makefile b/libio/Makefile index 6e594b8ec5..4a3637f046 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -66,11 +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-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 + tst-wfile-sync tests-internal = tst-vtables tst-vtables-interposed tst-readline @@ -173,12 +169,10 @@ 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-bz24228-ENV = MALLOC_TRACE=$(objpfx)tst-bz24228.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 @@ -194,8 +188,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-wfile-gconv-mem.out + $(objpfx)tst-bz22415-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. @@ -229,7 +222,6 @@ $(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 @@ -257,7 +249,3 @@ $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out $(objpfx)tst-bz24228-mem.out: $(objpfx)tst-bz24228.out $(common-objpfx)malloc/mtrace $(objpfx)tst-bz24228.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 c03c6cf57c..8a80dd0b78 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,14 +60,11 @@ _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; - 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); + + __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); } else { diff --git a/libio/tst-wfile-ascii.c b/libio/tst-wfile-ascii.c deleted file mode 100644 index 7514289a7b..0000000000 --- a/libio/tst-wfile-ascii.c +++ /dev/null @@ -1,56 +0,0 @@ -/* 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 deleted file mode 100644 index de603b32d2..0000000000 --- a/libio/tst-wfile-gconv.c +++ /dev/null @@ -1,36 +0,0 @@ -/* 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 840d4abc44..6648365d82 100644 --- a/wcsmbs/wcsmbsload.c +++ b/wcsmbs/wcsmbsload.c @@ -279,13 +279,3 @@ _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 c2fffbd914..6ccad4b3ba 100644 --- a/wcsmbs/wcsmbsload.h +++ b/wcsmbs/wcsmbsload.h @@ -51,7 +51,6 @@ 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>
The changes introduce a memory leak for gconv steps arrays whose first element is an internal conversion, which has a fixed reference count which is not decremented. As a result, after the change in commit 50ce3eae5ba304650459d4441d7d246a7cefc26f, the steps array is never freed, resulting in an unbounded memory leak. This reverts commit 50ce3eae5ba304650459d4441d7d246a7cefc26f ("gconv: Check reference count in __gconv_release_cache [BZ #24677]") and commit 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related memory leak [BZ #24583]"). It reintroduces bug 24583. (Bug 24677 was just a regression caused by the second commit.) 2019-07-31 Florian Weimer <fweimer@redhat.com> [BZ #24583] [BZ #24677] iconv, libio: Revert reference counting changes. * iconv/gconv_cache.c (__gconv_release_cache): Unconditionally free the steps array. * libio/Makefile (tests): Remove tst-wfile-gconv. (tests-container): Do not add tst-wfile-ascii. (tst-wfile-gconv-ENV): Do not set. (generated): Do not add tst-wfile-gconv.mtrace, tst-wfile-gconv.check. [($run-built-tests)] (tests-special): Do not add tst-wfile-gconv-mem.out. (tst-wfile-gconv.out, tst-wfile-gconv-mem.out): Remove targets. * libio/iofclose.c (_IO_new_fclose): Call __gconv_release_step instead of __wcsmbs_clone_conv. * wcsmbs/wcsmbsload.c (__wcsmbs_clone_conv): Remove definition. * wcsmbs/wcsmbsload.h (__wcsmbs_clone_conv): Remove declaration.