This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]
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 <fweimer@redhat.com>
[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 <fweimer@redhat.com>
[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 <carlos@redhat.com>
--
Cheers,
Carlos.