[PATCH] gconv: Check reference count in __gconv_release_cache [BZ #24677]

[Carlos O'Donell]

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.