From patchwork Wed Jul 31 09:50:20 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 1139658 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-104069-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="gW8aWr2Y"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45z7w85bKKz9sDQ for ; Wed, 31 Jul 2019 19:50:48 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id :mime-version:content-type; q=dns; s=default; b=R3RR/3dC5G6YdiUp Go9Qv+gQFkkn3qnkNO2WxCKoqXZIH1wm5EjsByLDO01BJvaIN2+MVWRIa7CXUMgw 8040tnmfd9ARN6BRliCgsmDsUEWaRQVADXp0A40CMeBh2mv49vs/r6bk+r0SUEpE MumC9GIEkUoILurrp8jeJCI/xLw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id :mime-version:content-type; s=default; bh=/C5QgcbrE4kT1RNdBt1jRg iBQyg=; b=gW8aWr2Y/5Y1wTNqfxA73y+S2H2m31LoC0Ugal+2o4jE7uIKX88jdd K2pyURwkfw2vwU94fnSqYF6iKf8wWjBZ44WHQg0Os5U/qD+SWKeWlPdLVThCJwYN opkit/MUAdZXwjharrEf62tdifuAUvpnOWLaNbNS62e+Jk26SPt4o= Received: (qmail 40553 invoked by alias); 31 Jul 2019 09:50:36 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 40463 invoked by uid 89); 31 Jul 2019 09:50:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=do_test X-HELO: mx1.redhat.com From: Florian Weimer To: libc-alpha@sourceware.org Cc: Carlos O'Donell Subject: [PATCH] iconv: Revert steps array reference counting changes Date: Wed, 31 Jul 2019 11:50:20 +0200 Message-ID: <87zhkur0rn.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 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. Reviewed-by: Carlos O'Donell 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