diff mbox

Fix BZ 19012 -- memory leak on error path in iconv_open

Message ID CAPC3xarLZpvmAwhyVj-v6C65QkepJMhSJGfZ2ZYQATXesm94fw@mail.gmail.com
State Changes Requested, archived
Headers

Commit Message

Paul Pluzhnikov Sept. 27, 2015, 9:22 p.m. UTC 
  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).

Thanks,

2015-09-27  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #19012]
        * iconv/gconv_db.c (gen_steps): Clean up on error.

Comments

Carlos O'Donell Sept. 28, 2015, 1:40 p.m. UTC | #1 
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;
>  		}
>
Florian Weimer Sept. 28, 2015, 1:53 p.m. UTC | #2 
On 09/27/2015 11:22 PM, Paul Pluzhnikov wrote:
> 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).

What happens if you call iconv in a loop?  Will the amount of memory
leaked remain bounded?
Mike Frysinger Sept. 28, 2015, 5:31 p.m. UTC | #3 
On 27 Sep 2015 14:22, Paul Pluzhnikov wrote:
> --- 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;
>  		}

what about the other failure case (init_fct returns an error) ?

the strduping seems a bit convoluted and makes error handling error prone.
what about something a bit more straightforward:
	char *to_name = NULL;
	char *from_name = NULL;
	...
	result[step_cnt].__from_name = (step_cnt == 0
	                                ? from_name = __strdup (fromset)
	                                : (char *)current->last->result_set);
	result[step_cnt].__to_name = (step_cnt + 1 == *nsteps
	                              ? fo_name = __strdup (current->result_set)
	                              : result[step_cnt + 1].__from_name);
	...

and then in the common clean up at the end:
	if (__builtin_expect (failed, 0) != 0) {
		...
		free (result);
		free (to_name);
		free (from_name);
		...
-mike
Paul Pluzhnikov Oct. 3, 2015, 5:14 p.m. UTC | #4 
On Mon, Sep 28, 2015 at 6:40 AM, Carlos O'Donell <carlos@redhat.com> wrote:

> However, your test program does not reproduce a leak for me when using master
> and removing SJIS.so.

It reproduces for me on current master if I remove SJIS.so *and*
gconv-modules.cache (which we also do not ship to the target to save
space).

It does not reproduce for me (on master or existing system EGLIBC
2.19-0ubuntu6.6) if remove only SJIS.so, but leave gconv-modules.cache
in place, so I think I've replicated your behavior.

Thanks,
Paul Pluzhnikov Oct. 3, 2015, 5:19 p.m. UTC | #5 
On Mon, Sep 28, 2015 at 6:53 AM, Florian Weimer <fweimer@redhat.com> wrote:

> What happens if you call iconv in a loop?  Will the amount of memory
> leaked remain bounded?

I did the following test:

#include <iconv.h>

int main() {
  int j;
  int n_failures = 0;

  for (j = 0; j < 10; ++j) {
    iconv_t res = iconv_open ("UTF8", "SJIS");
    if (res == (iconv_t) -1) {
      n_failures += 1;
    } else {
      iconv_close (res);
    }
  }
  return n_failures;
}

No leaks (and exit 0) when SJIS.so and gconv-modules.cache are unmolested.

When both are removed, only a single leak (and expected exit 10):

==130249== HEAP SUMMARY:
==130249==     in use at exit: 23 bytes in 2 blocks
==130249==   total heap usage: 2,362 allocs, 2,360 frees, 139,354
bytes allocated
==130249==
==130249== 7 bytes in 1 blocks are definitely lost in loss record 1 of 2
==130249==    at 0x4A06C3D: malloc (vg_replace_malloc.c:299)
==130249==    by 0x4C933C9: strdup (strdup.c:42)
==130249==    by 0x4C35B70: gen_steps (gconv_db.c:264)
==130249==    by 0x4C35B70: find_derivation (gconv_db.c:663)
==130249==    by 0x4C36386: __gconv_find_transform (gconv_db.c:764)
==130249==    by 0x4C34F84: __gconv_open (gconv_open.c:110)
==130249==    by 0x4C34B17: iconv_open (iconv_open.c:71)
==130249==    by 0x4005B3: main (t.c:8)
==130249==
==130249== 16 bytes in 1 blocks are definitely lost in loss record 2 of 2
==130249==    at 0x4A06C3D: malloc (vg_replace_malloc.c:299)
==130249==    by 0x4C933C9: strdup (strdup.c:42)
==130249==    by 0x4C35B82: gen_steps (gconv_db.c:267)
==130249==    by 0x4C35B82: find_derivation (gconv_db.c:663)
==130249==    by 0x4C36386: __gconv_find_transform (gconv_db.c:764)
==130249==    by 0x4C34F84: __gconv_open (gconv_open.c:110)
==130249==    by 0x4C34B17: iconv_open (iconv_open.c:71)
==130249==    by 0x4005B3: main (t.c:8)
==130249==
==130249== LEAK SUMMARY:
==130249==    definitely lost: 23 bytes in 2 blocks
==130249==    indirectly lost: 0 bytes in 0 blocks
==130249==      possibly lost: 0 bytes in 0 blocks
==130249==    still reachable: 0 bytes in 0 blocks
==130249==         suppressed: 0 bytes in 0 blocks
==130249==

So this isn't a very serious leak, but still.
Florian Weimer Oct. 3, 2015, 6:25 p.m. UTC | #6 
On 10/03/2015 07:19 PM, Paul Pluzhnikov wrote:
> On Mon, Sep 28, 2015 at 6:53 AM, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> What happens if you call iconv in a loop?  Will the amount of memory
>> leaked remain bounded?
> 
> I did the following test:
> 
> #include <iconv.h>
> 
> int main() {
>   int j;
>   int n_failures = 0;
> 
>   for (j = 0; j < 10; ++j) {
>     iconv_t res = iconv_open ("UTF8", "SJIS");
>     if (res == (iconv_t) -1) {
>       n_failures += 1;
>     } else {
>       iconv_close (res);
>     }
>   }
>   return n_failures;
> }
> 
> No leaks (and exit 0) when SJIS.so and gconv-modules.cache are unmolested.
> 
> When both are removed, only a single leak (and expected exit 10):

> So this isn't a very serious leak, but still.

Thanks for doing this experiment.  So it's not an unbounded memory leak,
and not a security vulnerability.

Florian
diff mbox

Patch

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