From patchwork Thu Jul 25 21:31:01 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 33800 Received: (qmail 42389 invoked by alias); 25 Jul 2019 21:31:08 -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 42370 invoked by uid 89); 25 Jul 2019 21:31:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=valgrind X-HELO: mx1.redhat.com From: Florian Weimer To: Carlos O'Donell Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677] References: <878ssvz52n.fsf@oldenburg2.str.redhat.com> <73f2c2c4-ad6b-faca-8707-44aa36cb08c9@redhat.com> Date: Thu, 25 Jul 2019 23:31:01 +0200 In-Reply-To: <73f2c2c4-ad6b-faca-8707-44aa36cb08c9@redhat.com> (Carlos O'Donell's message of "Thu, 25 Jul 2019 15:53:54 -0400") Message-ID: <871ryder8q.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 * 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: ==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. 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.) 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. Reviewed-by: Carlos O'Donell 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); }