Re: [patch] Fix BZ 19012 -- memory leak on error path in iconv_open

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

On 09/27/2015 05:22 PM, Paul Pluzhnikov wrote:
> Greetings,
> 
> Attached patch fixes the leak, but doesn't add a test case.
> 
> In order to expose the bug, a partial install (one missing
> iconvdata/SJIS.so) is required. I am not sure how to write such a test
> case (or even whether it is truly a bug to leak memory when faced with
> such a partial install).

Avoiding a memory leak results in cleaner valgrind runs without the need to
resort to custom supression lists. It also happens that the leak can be
fixed in a non-hot failure path (shlib_handle == NULL), therefore there is
much more room to detect failure, as opposed to the other discussion we were
having on list about corrupt locales (which should IMO fail catastrophically
and let the user know by aborting).

Therefore I think your patch is good in the sense that it detects a
misconfiguration and avoids the memory leak, while still continuging to return
the expected -1 from iconv_open.

However, your test program does not reproduce a leak for me when using master
and removing SJIS.so. Could you provide another test? Or confirm that you can
indeed reproduce the failure with master?

sudo mv /usr/lib64/gconv/SJIS.so /usr/lib64/gconv/SJIS.so.old
cat >> test.c << EOF
#include <iconv.h>

int main() {
  iconv_t res = iconv_open("UTF8", "SJIS");
  if (res == (iconv_t) -1) return 1;
  iconv_close(res);
  return 0;
}
EOF

BUILD=/home/carlos/build/glibc
gcc -Wall -pedantic -g3 -O0 -Wl,--dynamic-linker=$BUILD/elf/ld.so -Wl,-rpath=$BUILD -o test test.c

valgrind ./test
==7734== Memcheck, a memory error detector
==7734== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==7734== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==7734== Command: ./test
==7734== 
==7734== 
==7734== HEAP SUMMARY:
==7734==     in use at exit: 0 bytes in 0 blocks
==7734==   total heap usage: 6 allocs, 6 frees, 450 bytes allocated
==7734== 
==7734== All heap blocks were freed -- no leaks are possible
==7734== 
==7734== For counts of detected and suppressed errors, rerun with: -v
==7734== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Cheers,
Carlos.
 
> Thanks,
> 
> 2015-09-27  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #19012]
>         * iconv/gconv_db.c (gen_steps): Clean up on error.
> 
> -- Paul Pluzhnikov
> 
> 
> glibc-bz19012-20150927.txt
> 
> 
> diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
> index 4e6ec65..bb9a2bd 100644
> --- a/iconv/gconv_db.c
> +++ b/iconv/gconv_db.c
> @@ -279,6 +279,12 @@ gen_steps (struct derivation_step *best, const char *toset,
>  	      if (shlib_handle == NULL)
>  		{
>  		  failed = 1;
> +
> +		  /* Don't leak memory.  BZ #19012.  */
> +		  if (step_cnt == 0)
> +		    free (result[step_cnt].__from_name);
> +		  free (result[*nsteps - 1].__to_name);
> +
>  		  break;
>  		}
>