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.

[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.

diff --git a/libio/tst-wfile-fclose-exit.root/postclean.req b/libio/tst-wfile-fclose-exit.root/postclean.req
new file mode 100644
index 0000000000..e69de29bb2
  

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.

You'll have to confirm this works by hand and I'm OK with that for now.

This was quite complex to audit. The code manipulates a lot of distinct
members of structs without any clear rules of ownership or isolation.
This could all do with some kind of API or cleaner interface to handling
the functions instead of having abstract rules for how the elements are
handled. This was quite a mess. Thank you for the fix!

> 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);
>   }

(1) Taking __gconv_lock to protect __counter.

In __wcsmbs_clone_conv we use __gconv_lock to protect __counter.

In __gconv_release_cache we don't use __gconv_lock to protect __counter.

Only __gconv_close_transform calls __gconv_release_cache.

And __gconv_close_transform takes __gconv_lock so accessing __counter is OK.

(2) Distinct __counter fields.

I agree that the elements have distinct counter fields, and I double
checked that they will all be in sync, so that comment looks good, and
the logic makes sense.

Would love to see us make some sensible inline APIs like __*_in_use()
so we could remove all the step->__counter == 0 checks for something
that semantics.

OK.
    
> diff --git a/libio/Makefile b/libio/Makefile
> index 6e594b8ec5..3a4fb2f10e 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -68,9 +68,9 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
>   	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
> +# These tests interact with the gconv cache.  Setting GCONV_CACHE
> +# during out-of-container testing disables the cache.
> +tests-container += tst-wfile-ascii tst-wfile-fclose-exit

Please remove this.

This test doesn't work because of the converters being in the container
compressed and us not having any way in the container to decompress them.
I think we need a new install target for this so we can avoid the dependency
on gzip/bzip2, or we need to use compression that we bundle.

>   
>   tests-internal = tst-vtables tst-vtables-interposed tst-readline
>   
> @@ -230,6 +230,7 @@ $(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)
> +$(objpfx)tst-wfile-fclose-exit.out: $(gen-locales)
>   endif
>   
>   $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
> diff --git a/libio/tst-wfile-fclose-exit.c b/libio/tst-wfile-fclose-exit.c
> new file mode 100644
> index 0000000000..e9f19b5155
> --- /dev/null
> +++ b/libio/tst-wfile-fclose-exit.c

At this point in the release I'd like to see this fixed even without a test
case.

Please remove this test, test manually, and propose a test when 2.31 opens
with the proper fix, which is to install the decompressed converters in
a custom install target for testing (yes this is a deviation from what
we install normally, but it only changes some of the code being tested
e.g. charmap_open() and fopen_uncompressed()).

> @@ -0,0 +1,73 @@
> +/* Check that it is possible to call fclose on wide default streams.
> +   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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <locale.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +#include <wchar.h>
> +
> +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);
> +    free (iconvconfig);
> +  }
> +
> +  /* Build the en_US.UTF-8 locale.  */
> +  TEST_COMPARE (system ("localedef -f UTF-8 -i en_US en_US.UTF-8"), 0);
> +
> +  if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
> +    FAIL_EXIT1 ("setlocale: %m");
> +
> +  char *path;
> +  xclose (create_temp_file ("tst-wfile-fclose-exit-", &path));
> +  support_write_file_string (path, "test file\n");
> +
> +  /* Create a stream and put it into wide mode.  */
> +  FILE *fp = xfopen (path, "r");
> +  wint_t wc = fgetwc (fp);
> +  TEST_COMPARE (wc, 't');
> +
> +  /* Make sure that stdout is fully initialized and in wide mode.  */
> +  wprintf (L"info: character read: %d\n", (int) wc);
> +
> +  xfclose (fp);
> +  xfclose (stdin);
> +  xfclose (stdout);
> +  xfclose (stderr);
> +  free (path);
> +
> +  exit (0);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/libio/tst-wfile-fclose-exit.root/postclean.req b/libio/tst-wfile-fclose-exit.root/postclean.req
> new file mode 100644
> index 0000000000..e69de29bb2
>