Re: [bug-libextractor] [patch] fix textproc/libextractor double free() bug

[Christian Grothoff]

Patch looks good and will be in the next LE release (is already in subversion 
2237).  I'm still not sure under which circumstances the _destructor_ 
function is invoked twice without _constructor_ in the meantime (BSD-ism?) 
but the patch certainly makes the code cleaner in general.

So, thanks!

Christian [ libextractor maintainer ]

On Wednesday 09 November 2005 03:48, Vasil Dimov wrote:
> >Submitter-Id:        current-users
> >Originator:  Vasil Dimov
> >Organization:        DataMax
> >Confidential:        no
> >Synopsis:    [patch] textproc/libextractor fix double free() bug
> >Severity:    serious
> >Priority:    high
> >Category:    ports
> >Class:               sw-bug
> >Release:     FreeBSD 6.0-STABLE amd64
> >Environment:
>
> System: FreeBSD qlovarnika.bg.datamax 6.0-STABLE FreeBSD 6.0-STABLE #0: Tue
> Nov  8 09:24:59 EET 2005    
> address@hidden:/usr/obj/usr/src/sys/QLOVARNIKA  amd64
>
> >Description:
>
> Problem: `make check' for textproc/libextractor fails:
> ...
> make  check-TESTS
> lt-trivialtest in free(): error: chunk is already free
> Abort trap (core dumped)
> FAIL: trivialtest
> PASS: keywordlisttest
> lt-plugintest in free(): error: chunk is already free
> Abort trap (core dumped)
> FAIL: plugintest
> lt-multiload in free(): error: chunk is already free
> Abort trap (core dumped)
> FAIL: multiload
> =========================================
> 3 of 4 tests failed
> Please report to address@hidden
> =========================================
> ...
>
> (NOTE: options AJ are set via malloc.conf(3))
>
> Lets look closer to one of the failing progs:
>
> $ cd libextractor-0.5.6a/src/test
> $ ./.libs/lt-trivialtest
> lt-trivialtest in free(): error: chunk is already free
> Abort trap: 6 (core dumped)
> $
>
> gdb is useless here, because it segfaults itself, so I used printfs to
> locate the problem.
>
> the following is self-explanatory trace of the program, created by
> inserting printfs in the appropriate places:
> (the prog is being run via electricfence to make it crash as soon as
> possible)
>
> $ ./.libs/lt-trivialtest
> extractor.c:216 le_ltdl_init begin
> extractor.c:253 le_ltdl_init end
> trivialtest.c:24 main begin
> extractor.c:216 le_ltdl_init begin
> extractor.c:235 le_ltdl_init old_dlsearchpath=0x801122fcc
> extractor.c:237 le_ltdl_init
> old_dlsearchpath=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:253 le_ltdl_init end
> extractor.c:258 le_ltdl_fini begin
> ltdl.c:4058 lt_dlsetsearchpath search_path=0x801122fcc
> ltdl.c:4060 lt_dlsetsearchpath
> search_path=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:274 le_ltdl_fini end
> trivialtest.c:28 main between
> extractor.c:216 le_ltdl_init begin
> extractor.c:235 le_ltdl_init old_dlsearchpath=0x802cd3fcc
> extractor.c:237 le_ltdl_init
> old_dlsearchpath=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:253 le_ltdl_init end
> extractor.c:258 le_ltdl_fini begin
> ltdl.c:4058 lt_dlsetsearchpath search_path=0x802cd3fcc
> ltdl.c:4060 lt_dlsetsearchpath
> search_path=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:274 le_ltdl_fini end
> trivialtest.c:31 main end
> extractor.c:258 le_ltdl_fini begin
> ltdl.c:4058 lt_dlsetsearchpath search_path=0x802cd3fcc
> Bus error: 10 (core dumped)
> $
>
> You see, the destructor le_ltdl_fini() is being called "unexpectedly" when
> the program exits, but search_path=0x802cd3fcc has already been free()d by
> the previous invocation.
>
> Fix: it seems quite trivial, we just need to old_dlsearchpath = NULL
> after free()ing it - lt_dlsetsearchpath() is checking if called with NULL
> pointer and we are calling free() only if old_dlsearchpath is non-NULL.
>
> With the included patch, output of trivialtest looks like:
>
> $ ./.libs/lt-trivialtest
> extractor.c:216 le_ltdl_init begin
> extractor.c:253 le_ltdl_init end
> trivialtest.c:24 main begin
> extractor.c:216 le_ltdl_init begin
> extractor.c:235 le_ltdl_init old_dlsearchpath=0x801122fcc
> extractor.c:237 le_ltdl_init
> old_dlsearchpath=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:253 le_ltdl_init end
> extractor.c:258 le_ltdl_fini begin
> ltdl.c:4058 lt_dlsetsearchpath search_path=0x801122fcc
> ltdl.c:4060 lt_dlsetsearchpath
> search_path=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:274 le_ltdl_fini end
> trivialtest.c:28 main between
> extractor.c:216 le_ltdl_init begin
> extractor.c:235 le_ltdl_init old_dlsearchpath=0x802cd3fcc
> extractor.c:237 le_ltdl_init
> old_dlsearchpath=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:253 le_ltdl_init end
> extractor.c:258 le_ltdl_fini begin
> ltdl.c:4058 lt_dlsetsearchpath search_path=0x802cd3fcc
> ltdl.c:4060 lt_dlsetsearchpath
> search_path=/usr/lib/libextractor:/usr/local/lib/libextractor
> extractor.c:274 le_ltdl_fini end
> trivialtest.c:31 main end
> extractor.c:258 le_ltdl_fini begin
> ltdl.c:4058 lt_dlsetsearchpath search_path=0x0
> ltdl.c:4060 lt_dlsetsearchpath search_path=(null)
> extractor.c:274 le_ltdl_fini end
> $
>
> And of-course...
> $ make check
> ...
> ==================
> All 4 tests passed
> ==================
> $
>
> >How-To-Repeat:
>
> export MALLOC_OPTIONS=AJ
> cd /usr/ports/textproc/libextractor
> make check
>
> >Fix:
>
> --- libextractor_free.diff begins here ---
> diff -urN --exclude=CVS libextractor.orig/Makefile libextractor/Makefile
> --- libextractor.orig/Makefile        Sun Nov  6 02:20:45 2005
> +++ libextractor/Makefile     Wed Nov  9 12:55:44 2005
> @@ -7,7 +7,7 @@
>
>  PORTNAME=    libextractor
>  PORTVERSION= 0.5.6a
> -PORTREVISION=        1
> +PORTREVISION=        2
>  CATEGORIES=  textproc
>  MASTER_SITES=        http://gnunet.org/libextractor/download/ \
>               ${MASTER_SITE_GNU}
> diff -urN --exclude=CVS
> libextractor.orig/files/patch-src::main::extractor.c
> libextractor/files/patch-src::main::extractor.c ---
> libextractor.orig/files/patch-src::main::extractor.c  Thu Jan  1 02:00:00
> 1970 +++ libextractor/files/patch-src::main::extractor.c      Wed Nov  9
> 12:02:48 2005 @@ -0,0 +1,14 @@
> +--- src/main/extractor.c.orig        Wed Nov  9 11:57:54 2005
> ++++ src/main/extractor.c     Wed Nov  9 11:58:31 2005
> +@@ -246,8 +246,10 @@
> +
> + void __attribute__ ((destructor)) le_ltdl_fini(void) {
> +   lt_dlsetsearchpath(old_dlsearchpath);
> +-  if (old_dlsearchpath != NULL)
> ++  if (old_dlsearchpath != NULL) {
> +     free(old_dlsearchpath);
> ++    old_dlsearchpath = NULL;
> ++  }
> + #ifdef MINGW
> +   ShutdownWinEnv();
> + #endif
> --- libextractor_free.diff ends here ---
>
>
> _______________________________________________
> bug-libextractor mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/bug-libextractor