Ensembl / Bio-DB-HTS

Git repo for Bio::DB::HTS module on CPAN, providing Perl links into HTSlib
Apache License 2.0
24 stars 16 forks source link

Add memory leak cases to test suite #66

Closed avullo closed 6 years ago

avullo commented 6 years ago

The latest, more comprehensive, travis builds have exposed some memory leaks which occur when releasing memory of an index. Ensure these and other types do not occur by adding appropriate tests, e.g. using Test::LeakTrace.

avullo commented 6 years ago

@jmarshall I've added this but it doesn't complain, I suspect that's because it's just testing at the perl API layer whether there are leaked SVs, which isn't covering the double-free case.

jmarshall commented 6 years ago

I'm seeing this with:

$ cd $HOME/Bio-DB-HTS
$ valgrind --suppressions=$HOME/suppressions/perl.supp --leak-check=full perl -I$HOME/Bio-DB-HTS/_build/lib -I/path/to/BioPerl/1.7.1/lib/perl5/x86_64-linux-thread-multi -I/path/to/BioPerl/1.7.1/lib/perl5 t/03cram.t 
==7813== Memcheck, a memory error detector
==7813== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==7813== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==7813== Command: […]
==7813== 
1..139
# Running under perl version 5.010001 for linux
[…]
ok 54
==7813== Invalid read of size 8
==7813==    at 0xFBAA247: cram_index_free (cram_index.c:326)
==7813==    by 0xFB57E51: hts_idx_destroy (hts.c:1597)
==7813==    by 0xF91E803: XS_Bio__DB__HTS__Index_DESTROY (HTS.xs:1264)
[…]
==7813==  Address 0xe44aad8 is 34,888 bytes inside a block of size 35,136 free'd
==7813==    at 0x4C27430: free (vg_replace_malloc.c:446)
==7813==    by 0xFBB6B2F: cram_close (cram_io.c:4329)
==7813==    by 0xFB5A938: hts_close (hts.c:918)
==7813==    by 0xF91A461: XS_Bio__DB__HTSfile_DESTROY (HTS.xs:493)
[…]

As you see, this was with an ancient (CentOS 6) Perl 5.10.1. I've seen it also with 5.24. However the order in which things happen to get destroyed might be quite Perl version and environment dependent…

avullo commented 6 years ago

Thanks @jmarshall I guess you observe the offense whenever you try to execute that, and not just when you get the segfault.

To add to this, I've run with prove but you did by invoking perl directly, and it does make a difference. Now I can observe something similar: lots of invalid read messages, but only involving either read1 or _index_build or _index_fetch.

jmarshall commented 6 years ago

Hmmm… I take it back, with my instrumented code I don't see this in Perl 5.24; instead of the valgrind use-after-free message I get an extra leak still present at the end:

==20437== 86 bytes in 2 blocks are possibly lost in loss record 1,375 of 2,395
==20437==    at 0x4C27A2E: malloc (vg_replace_malloc.c:270)
==20437==    by 0x4EFE444: Perl_refcounted_he_new_pvn (in /opt/rh/rh-perl524/root/usr/lib64/libperl.so.rh-perl524-5.24.0)
==20437==    by 0x4EFE840: Perl_refcounted_he_new_sv (in /opt/rh/rh-perl524/root/usr/lib64/libperl.so.rh-perl524-5.24.0)

On Perl 5.10.1, yes I see the use-after-free every time, but it's only ended in a segfault once out of thirty or so runs.

I'm running perl directly because I like to cut things down to the minimum when running under valgrind. It shouldn't make too much difference to the observations.

Anyway, as I'm the one who can observe this problem easily, I'm happy to try to fix it. Or you might try in a sandbox with some more ancient Perls (maybe the 5.14 on travis?)…

avullo commented 6 years ago

@jmarshall I'm installing 5.10.1, let me try and if successful I can free you from the burden. Adding some other older perls to travis won't hurt ;) It would also be good to have the entire distribution under the supervision of cpantesters, but I don't remember the reason why @rishidev decided to leave it out (might be the dependence on bioperl, I suspect).

jmarshall commented 6 years ago

Cool, thanks. The debugging printfs in the wwcrc/i67-logging branch may be of interest.