Re: [PATCH] iconv: Revert steps array reference counting changes

[Florian Weimer <fweimer at redhat dot com>]

* Carlos O'Donell:

> On 7/31/19 5:50 AM, Florian Weimer wrote:
>> 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 50ce3eae5ba304650459d4441d7d246a7cefc26f, the steps
>> array is never freed, resulting in an unbounded memory leak.
>
> If you have done the analysis and the result is indeed an unbounded
> memory leak then yes, we should revert this back to the original state.
> I'm curious how it happens, but we can talk about that in the coming
> weeks. There must be something about the wide usage that I don't follow.

I think we populate the array with reference count 1, see
__gconv_load_cache:

	      result[idx].__counter = 1;
	      result[idx].__data = NULL;

	      if (strtab[extra->module[idx].dir_offset] != '\0')
…
	      else
		/* It's a builtin transformation.  */
		__gconv_get_builtin_trans (strtab
					   + extra->module[idx].name_offset,
					   &result[idx]);

__gconv_get_builtin_trans does not updated __counter.

But __gconv_release_step looks does this:

void
__gconv_release_step (struct __gconv_step *step)
{
  /* Skip builtin modules; they are not reference counted.  */
  if (step->__shlib_handle != NULL && --step->__counter == 0)

So we never lower the reference count to zero.  That's also what I saw
in the debugger.  Since it impacts iconv_open/iconv_close and the
allocation is stored only in the iconv handle itself, it's automatically
unbounded.

I pushed this and updated Bugzilla.  Please do another build to check
that the tree is fine.  It's very late here.

Thanks,
Florian