iconv: Revert steps array reference counting changes

The changes introduce a memory leak for gconv steps arrays whose
first element is an internal conversion, which has a fixed
reference count which is not decremented.  As a result, after the
change in commit 50ce3ea, the steps
array is never freed, resulting in an unbounded memory leak.

This reverts commit 50ce3ea
("gconv: Check reference count in __gconv_release_cache
[BZ #24677]") and commit 7e740ab
("libio: Fix gconv-related memory leak [BZ #24583]").  It
reintroduces bug 24583.  (Bug 24677 was just a regression caused by
the second commit.)

ChangeLog

@@ -1,3 +1,23 @@
+2019-07-31  Florian Weimer  <[email protected]>
+
+	[BZ #24583]
+	[BZ #24677]
+	iconv, libio: Revert reference counting changes.
+	* iconv/gconv_cache.c (__gconv_release_cache): Unconditionally
+	free the steps array.
+	* libio/Makefile (tests): Remove tst-wfile-gconv.
+	(tests-container): Do not add tst-wfile-ascii.
+	(tst-wfile-gconv-ENV): Do not set.
+	(generated): Do not add tst-wfile-gconv.mtrace,
+	tst-wfile-gconv.check.
+	[($run-built-tests)] (tests-special): Do not add
+	tst-wfile-gconv-mem.out.
+	(tst-wfile-gconv.out, tst-wfile-gconv-mem.out): Remove targets.
+	* libio/iofclose.c (_IO_new_fclose): Call __gconv_release_step
+	instead of __wcsmbs_clone_conv.
+	* wcsmbs/wcsmbsload.c (__wcsmbs_clone_conv): Remove definition.
+	* wcsmbs/wcsmbsload.h (__wcsmbs_clone_conv): Remove declaration.
+
 2019-07-30  Joseph Myers  <[email protected]>
 
 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/swapcontext-common.S

iconv/gconv_cache.c

@@ -446,12 +446,9 @@ __gconv_lookup_cache (const char *toset, const char *fromset,
 void
 __gconv_release_cache (struct __gconv_step *steps, size_t nsteps)
 {
-  /* 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)
+  if (gconv_cache != NULL)
+    /* The only thing we have to deallocate is the record with the
+       steps.  */
     free (steps);
 }
 

libio/Makefile

@@ -66,11 +66,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
 	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
 	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
 	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
+	tst-wfile-sync
 
 tests-internal = tst-vtables tst-vtables-interposed tst-readline
 
@@ -173,12 +169,10 @@ test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace
 tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace
 tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace
 tst-bz24228-ENV = MALLOC_TRACE=$(objpfx)tst-bz24228.mtrace
-tst-wfile-gconv-ENV = MALLOC_TRACE=$(objpfx)tst-wfile-gconv.mtrace
 
 generated += test-fmemopen.mtrace test-fmemopen.check
 generated += tst-fopenloc.mtrace tst-fopenloc.check
 generated += tst-bz22415.mtrace tst-bz22415.check
-generated += tst-wfile-gconv.mtrace tst-wfile-gconv.check
 
 aux	:= fileops genops stdfiles stdio strops
 
@@ -194,8 +188,7 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops	\
 
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \
-		 $(objpfx)tst-bz22415-mem.out \
-		 $(objpfx)tst-wfile-gconv-mem.out
+		 $(objpfx)tst-bz22415-mem.out
 ifeq (yes,$(build-shared))
 # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared
 # library is enabled since they depend on tst-fopenloc.out.
@@ -229,7 +222,6 @@ $(objpfx)tst-ungetwc2.out: $(gen-locales)
 $(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)
 endif
 
 $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
@@ -257,7 +249,3 @@ $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out
 $(objpfx)tst-bz24228-mem.out: $(objpfx)tst-bz24228.out
 	$(common-objpfx)malloc/mtrace $(objpfx)tst-bz24228.mtrace > $@; \
 	$(evaluate-test)
-
-$(objpfx)tst-wfile-gconv-mem.out: $(objpfx)tst-wfile-gconv.out
-	$(common-objpfx)malloc/mtrace $(objpfx)tst-wfile-gconv.mtrace > $@; \
-	$(evaluate-test)

libio/iofclose.c

@@ -26,8 +26,8 @@
 
 #include "libioP.h"
 #include <stdlib.h>
+#include "../iconv/gconv_int.h"
 #include <shlib-compat.h>
-#include <wcsmbs/wcsmbsload.h>
 
 int
 _IO_new_fclose (FILE *fp)
@@ -60,14 +60,11 @@ _IO_new_fclose (FILE *fp)
       /* This stream has a wide orientation.  This means we have to free
 	 the conversion functions.  */
       struct _IO_codecvt *cc = fp->_codecvt;
-      struct gconv_fcts conv =
-	{
-	 .towc = cc->__cd_in.__cd.__steps,
-	 .towc_nsteps = cc->__cd_in.__cd.__nsteps,
-	 .tomb = cc->__cd_out.__cd.__steps,
-	 .tomb_nsteps = cc->__cd_out.__cd.__nsteps,
-	};
-      __wcsmbs_close_conv (&conv);
+
+      __libc_lock_lock (__gconv_lock);
+      __gconv_release_step (cc->__cd_in.__cd.__steps);
+      __gconv_release_step (cc->__cd_out.__cd.__steps);
+      __libc_lock_unlock (__gconv_lock);
     }
   else
     {

wcsmbs/wcsmbsload.c

@@ -279,13 +279,3 @@ _nl_cleanup_ctype (struct __locale_data *locale)
       free ((char *) data);
     }
 }
-
-/* Free the specified conversion functions (but not CONV itself).  */
-void
-__wcsmbs_close_conv (struct gconv_fcts *conv)
-{
-  if (conv->towc != &to_wc)
-      __gconv_close_transform (conv->towc, conv->towc_nsteps);
-  if (conv->tomb != &to_mb)
-      __gconv_close_transform (conv->tomb, conv->tomb_nsteps);
-}

wcsmbs/wcsmbsload.h

@@ -51,7 +51,6 @@ extern int __wcsmbs_named_conv (struct gconv_fcts *copy, const char *name)
 /* Function used for the `private.cleanup' hook.  */
 extern void _nl_cleanup_ctype (struct __locale_data *) attribute_hidden;
 
-extern void __wcsmbs_close_conv (struct gconv_fcts *conv) attribute_hidden;
 
 #include <iconv/gconv_int.h>