Message ID | CAPC3xarLZpvmAwhyVj-v6C65QkepJMhSJGfZ2ZYQATXesm94fw@mail.gmail.com |
---|---|
State | New |
Headers | show |
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; > } >
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?
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
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,
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.
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 --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; }