From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 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 dcvr.yhbt.net (Postfix) with ESMTPS id 32FFE1F462 for ; Thu, 25 Jul 2019 21:41:57 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=xPJqNqFZyvlWlEX4 lemq9TZQMqzhA8K2iHmi30HZ8B4NEwGImP3XjTfzJF6eoCsutSPOLc/qSAP1LbA+ AhHUog4SfuphVCaFxTP6bcyXfN1enK74zsBTnXtDoTGfUL33q4DEOl1sfnqDE2JH n30UcJ7HkWpVk4wdCVjsbY+3t00= 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:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=H04/KfOmBPk4lPmgHdZU7G kRcc8=; b=WJh+cemMeukPc9fQ4zRxMUuYZPrbDdRkEdV69d4JL0pyIkPKNVOkS+ pv3wiFM7gPoZG+pWFdYlBAfSZGnb86pUSBoupbYqoxTMEXV6+8tm17gbdaUqzxfC KEvY4lNnBh8pIVmBs8L/y6C5x4PKlsXmOsAVfqPCmFLkqoUdyY+Yg= Received: (qmail 53691 invoked by alias); 25 Jul 2019 21:41:54 -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 Received: (qmail 53683 invoked by uid 89); 25 Jul 2019 21:41:54 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: mail-qt1-f193.google.com Subject: Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677] To: Florian Weimer Cc: libc-alpha@sourceware.org References: <878ssvz52n.fsf@oldenburg2.str.redhat.com> <73f2c2c4-ad6b-faca-8707-44aa36cb08c9@redhat.com> <871ryder8q.fsf@oldenburg2.str.redhat.com> From: Carlos O'Donell Message-ID: <3a12d58f-09b9-575c-2137-5da431da3123@redhat.com> Date: Thu, 25 Jul 2019 17:41:48 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <871ryder8q.fsf@oldenburg2.str.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 7/25/19 5:31 PM, Florian Weimer wrote: > * Carlos O'Donell: > >> On 7/18/19 12:21 PM, Florian Weimer wrote: >>> This fixes a regression introduced in commit >>> 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related >>> memory leak [BZ #24583]"). >>> >>> __gconv_release_cache is only ever called with heap-allocated >>> arrays which contain at least one member. The statically allocated >>> ASCII steps are filtered out by __wcsmbs_close_conv. >>> >>> [This variant cleans up __gconv_release_cache somewhat and includes the >>> test modification suggested by Yann. >>> >>> The test does not work due to bug 24824. I think installing all locales >>> unconditionally is too costly timewise. Suggestions how to proceed are >>> very welcome.] >>> >>> 2019-07-18 Florian Weimer >>> >>> [BZ #24677] >>> * iconv/gconv_cache.c (__gconv_release_cache): Check reference >>> counter before freeing array. >>> * libio/Makefile (tests-container): Add tst-wfile-fclose-exit. >>> * libio/tst-wfile-fclose-exit.c: New file. >>> * libio/tst-wfile-fclose-exit.root/postclean.req: Likewise. >> >> Please post a v2 without a test case. > > Thanks. Please see below. > >> You'll have to confirm this works by hand and I'm OK with that for >> now. > > valgrind is clean, but __libc_freeres is not fully effective: I expected something like this would happen. > ==2503== 208 bytes in 1 blocks are still reachable in loss record 1 of 2 > ==2503== at 0x480B80B: malloc (vg_replace_malloc.c:309) > ==2503== by 0x4847876: __gconv_lookup_cache (gconv_cache.c:366) > ==2503== by 0x483F9A1: __gconv_find_transform (gconv_db.c:733) > ==2503== by 0x48C8847: __wcsmbs_getfct (wcsmbsload.c:92) > ==2503== by 0x48C89A9: __wcsmbs_load_conv (wcsmbsload.c:186) > ==2503== by 0x48C8C26: get_gconv_fcts (wcsmbsload.h:75) > ==2503== by 0x48C8C26: __wcsmbs_clone_conv (wcsmbsload.c:222) > ==2503== by 0x4890B50: _IO_fwide (iofwide.c:79) > ==2503== by 0x488E359: __wuflow (wgenops.c:225) > ==2503== by 0x488C447: getwc (getwc.c:39) > ==2503== by 0x402581: do_test (tst-wfile-fclose-exit.c:45) > ==2503== by 0x403044: support_test_main (support_test_main.c:350) > ==2503== by 0x4023C4: main (test-driver.c:168) > ==2503== > ==2503== 208 bytes in 1 blocks are still reachable in loss record 2 of 2 > ==2503== at 0x480B80B: malloc (vg_replace_malloc.c:309) > ==2503== by 0x48477AE: __gconv_lookup_cache (gconv_cache.c:366) > ==2503== by 0x483F9A1: __gconv_find_transform (gconv_db.c:733) > ==2503== by 0x48C8847: __wcsmbs_getfct (wcsmbsload.c:92) > ==2503== by 0x48C89C6: __wcsmbs_load_conv (wcsmbsload.c:189) > ==2503== by 0x48C8C26: get_gconv_fcts (wcsmbsload.h:75) > ==2503== by 0x48C8C26: __wcsmbs_clone_conv (wcsmbsload.c:222) > ==2503== by 0x4890B50: _IO_fwide (iofwide.c:79) > ==2503== by 0x488E359: __wuflow (wgenops.c:225) > ==2503== by 0x488C447: getwc (getwc.c:39) > ==2503== by 0x402581: do_test (tst-wfile-fclose-exit.c:45) > ==2503== by 0x403044: support_test_main (support_test_main.c:350) > ==2503== by 0x4023C4: main (test-driver.c:168) > > Note that this leak is only visibile with --leak-check=full > --show-reachable, and only affects programs which use wide streams in > certain ways. That's fine. Thanks for checking. It's much better than double-free. > So we may have to tweak the code that frees the gconv cache. This is > quite risky because at that point, the steps array is still referenced > from the current locale. For glibc 2.31, we should perhaps switch to > the C locale at the start of __libc_free as a fix. (In glibc 2.29 and > earlier, I believe we free the gconv cache even though its entries are > still in use.) Yes, I assume you mean __libc_freeres? Yes, in __libc_freeres we can do all sorts of things like switch to the C locale, and then free all the objects. > Florian > > gconv: Check reference count in __gconv_release_cache [BZ #24677] > > This fixes a regression introduced in commit > 7e740ab2e7be7d83b75513aa406e0b10875f7f9c ("libio: Fix gconv-related > memory leak [BZ #24583]"). > > __gconv_release_cache is only ever called with heap-allocated > arrays which contain at least one member. The statically allocated > ASCII steps are filtered out by __wcsmbs_close_conv. > > 2019-07-25 Florian Weimer > > [BZ #24677] > * iconv/gconv_cache.c (__gconv_release_cache): Check reference > counter before freeing array. > > diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c > index 9a456bf825..4db7287cee 100644 > --- a/iconv/gconv_cache.c > +++ b/iconv/gconv_cache.c > @@ -446,9 +446,12 @@ __gconv_lookup_cache (const char *toset, const char *fromset, > void > __gconv_release_cache (struct __gconv_step *steps, size_t nsteps) > { > - if (gconv_cache != NULL) > - /* The only thing we have to deallocate is the record with the > - steps. */ > + /* 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) > free (steps); > } > > OK. Please commit to master. Reviewed-by: Carlos O'Donell -- Cheers, Carlos.