From a4d24cbe4552a79c9f854fc308bb01afcd7f5b8e Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Wed, 31 Jul 2019 11:53:38 +0200 Subject: [PATCH] Fix memory leak in iconv_open (#1734680) --- glibc-rh1734680.patch | 279 ++++++++++++++++++++++++++++++++++++++++++ glibc.spec | 7 +- 2 files changed, 285 insertions(+), 1 deletion(-) create mode 100644 glibc-rh1734680.patch diff --git a/glibc-rh1734680.patch b/glibc-rh1734680.patch new file mode 100644 index 0000000..c4b5137 --- /dev/null +++ b/glibc-rh1734680.patch @@ -0,0 +1,279 @@ +iconv: Revert steps array reference counting changes + +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 + + [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 ++#include "../iconv/gconv_int.h" + #include +-#include + + 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 +- . */ +- +-#include +-#include +-#include +-#include +-#include +- +-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 +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 +- . */ +- +-#include +-#include +-#include +-#include +- +-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 +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 + diff --git a/glibc.spec b/glibc.spec index d1177eb..44db764 100644 --- a/glibc.spec +++ b/glibc.spec @@ -87,7 +87,7 @@ Summary: The GNU libc libraries Name: glibc Version: %{glibcversion} -Release: 36%{?dist} +Release: 37%{?dist} # In general, GPLv2+ is used by programs, LGPLv2+ is used for # libraries. @@ -160,6 +160,8 @@ Patch28: glibc-rh1615608.patch # https://www.sourceware.org/ml/libc-alpha/2019-03/msg00436.html Patch31: glibc-fedora-nscd-warnings.patch +Patch32: glibc-rh1734680.patch + ############################################################################## # Continued list of core "glibc" package information: ############################################################################## @@ -2015,6 +2017,9 @@ fi %files -f compat-libpthread-nonshared.filelist -n compat-libpthread-nonshared %changelog +* Wed Jul 31 2019 Florian Weimer - 2.29.9000-37 +- Fix memory leak in iconv_open (#1734680) + * Tue Jul 30 2019 Florian Weimer - 2.29.9000-36 - Drop glibc-rh1732406.patch, fix for the regression applied upstream. - Auto-sync with upstream branch master,