[Crash-utility] [PATCH 2/2] Fix for memory leaks

[Wei, Jiangang]

On Mon, 2015-04-27 at 11:56 -0400, Dave Anderson wrote:
> 
> ----- Original Message -----
> > Without this patch, the storage reserved with the call
> > to strdup won't be freed.
> 
> Did you even test this patch?  It causes this:
> 
>  $ patch -p1 < /tmp/leaks.patch
>  patching file configure.c
>  Hunk #1 succeeded at 768 (offset 64 lines).
>  Hunk #2 succeeded at 1723 (offset 75 lines).
> 
>  $ make
>  *** glibc detected *** ./configure: munmap_chunk(): invalid pointer: 0x0000000000404e50 ***
>  ======= Backtrace: =========
>  /lib64/libc.so.6[0x3e43e7ae16]
>  ./configure[0x401e77]
>  ./configure[0x400edd]
>  /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e43e21735]
>  ./configure[0x400d89]
>  ======= Memory map: ========
>  00400000-00407000 r-xp 00000000 fd:01 167898                             /var/CVS/crash-7.1.0/configure
>  00607000-00608000 rw-p 00007000 fd:01 167898                             /var/CVS/crash-7.1.0/configure
>  0184e000-0186f000 rw-p 00000000 00:00 0                                  [heap]
>  3e43a00000-3e43a20000 r-xp 00000000 fd:01 1836136                        /usr/lib64/ld-2.15.so
>  3e43c1f000-3e43c20000 r--p 0001f000 fd:01 1836136                        /usr/lib64/ld-2.15.so
>  3e43c20000-3e43c21000 rw-p 00020000 fd:01 1836136                        /usr/lib64/ld-2.15.so
>  3e43c21000-3e43c22000 rw-p 00000000 00:00 0 
>  3e43e00000-3e43fac000 r-xp 00000000 fd:01 1836137                        /usr/lib64/libc-2.15.so
>  3e43fac000-3e441ac000 ---p 001ac000 fd:01 1836137                        /usr/lib64/libc-2.15.so
>  3e441ac000-3e441b0000 r--p 001ac000 fd:01 1836137                        /usr/lib64/libc-2.15.so
>  3e441b0000-3e441b2000 rw-p 001b0000 fd:01 1836137                        /usr/lib64/libc-2.15.so
>  3e441b2000-3e441b7000 rw-p 00000000 00:00 0 
>  3e46a00000-3e46a15000 r-xp 00000000 fd:01 1853385                        /usr/lib64/libgcc_s-4.7.2-20120921.so.1
>  3e46a15000-3e46c14000 ---p 00015000 fd:01 1853385                        /usr/lib64/libgcc_s-4.7.2-20120921.so.1
>  3e46c14000-3e46c15000 rw-p 00014000 fd:01 1853385                        /usr/lib64/libgcc_s-4.7.2-20120921.so.1
>  7ff116e8b000-7ff116e8e000 rw-p 00000000 00:00 0 
>  7ff116ea7000-7ff116eab000 rw-p 00000000 00:00 0 
>  7fff84a52000-7fff84a73000 rw-p 00000000 00:00 0                          [stack]
>  7fff84b0c000-7fff84b0e000 r-xp 00000000 00:00 0                          [vdso]
>  ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
>  make: *** [all] Aborted (core dumped)
>  $
> 
> It crashes because gdb_conf_flags is initialized to one of several possible
> built-in strings before it is passed to get_extra_flags(): 
> 
>  void
>  build_configure(struct supported_gdb_version *sp)
>  {
>          FILE *fp1, *fp2;
>          char buf[512];
>          char *target;
>          char *target_CFLAGS;
>          char *gdb_conf_flags;
>          char *ldflags;
>          char *cflags;
> 
>          get_current_configuration(sp);
> 
>          target = target_CFLAGS = NULL;
> 
>          gdb_conf_flags = GDB_TARGET_DEFAULT;
> 
>  ... [ cut ] ...
>          or it may be set to one of these: 
> 
> 			gdb_conf_flags = GDB_TARGET_X86_ON_X86_64;
> 			gdb_conf_flags = GDB_TARGET_PPC_ON_PPC64;
> 			gdb_conf_flags = GDB_TARGET_PPC64_ON_X86_64;
> 			gdb_conf_flags = GDB_TARGET_ARM_ON_X86;
> 			gdb_conf_flags = GDB_TARGET_ARM_ON_X86_64;
> 			gdb_conf_flags = GDB_TARGET_ARM64_ON_X86_64;
> 			gdb_conf_flags = GDB_TARGET_MIPS_ON_X86;
> 			gdb_conf_flags = GDB_TARGET_MIPS_ON_X86_64;
>  ... [ cut ] ...
> 
>          gdb_conf_flags = get_extra_flags("GDBFLAGS.extra", gdb_conf_flags); 
> 
> 
> And in the normal case where "GDBFLAGS.extra" does not exist, the hardwired string is 
> passed back, and then the crash occurs when your patch tries to free it:
> 
>  char *
>  get_extra_flags(char *filename, char *initial)
>  {
>          FILE *fp;
>          char inbuf[512];
>          char buf[512];
> 
>          if (!file_exists(filename))
>                  return (initial ? initial : NULL);
>  ...
> 
> Since configure.c simply runs and exits immediately when "make" is entered, it's hard to
> qualify these as serious memory leaks.  They fall more under the category of "who cares?".  

OK, 
Thanks for your reply and explanation.
and I think your comment is reasonable.
Please ignore this one.

Thanks,
wei
> 
> Dave
> 
> 
> > 
> > Signed-off-by: Wei,Jiangang <weijg.fnst at cn.fujitsu.com>
> > ---
> >  configure.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/configure.c b/configure.c
> > index 77ac81d..f78ad61 100755
> > --- a/configure.c
> > +++ b/configure.c
> > @@ -704,6 +704,10 @@ build_configure(struct supported_gdb_version *sp)
> >  
> >  	}
> >  
> > +	free(ldflags);
> > +	free(cflags);
> > +	free(gdb_conf_flags);
> > +
> >  	makefile_create(&fp1, &fp2);
> >  	show_configuration();
> >  	make_build_data(&target[strlen("TARGET=")]);
> > @@ -1644,6 +1648,9 @@ add_extra_lib(char *option)
> >  			add_lsnappy++;
> >  	}
> >  
> > +	free(ldflags);
> > +	free(cflags);
> > +
> >  	if ((lzo || snappy) &&
> >  	    file_exists("diskdump.o") && (unlink("diskdump.o") < 0)) {
> >  		perror("diskdump.o");
> > --
> > 1.9.3
> > 
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://www.redhat.com/mailman/listinfo/crash-utility
> > 
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility