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

[Carlos O'Donell <carlos at redhat dot com>]

On 7/31/19 5:11 PM, Florian Weimer wrote:
> * 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.

I did an x86_64 and i686 build with testing and it was fine.

--
Cheers,
Carlos.