iqbal-lab / Mykrobe-predictor

Antibiotic resistance predictions in minutes on a laptop
Other
50 stars 19 forks source link

(Trivial) Memory leaks in new species code #45

Closed Phelimb closed 8 years ago

Phelimb commented 9 years ago

Moving https://github.com/iqbal-lab/mykrobe_paper/issues/60

"There's a function called create_mask which does a malloc and returns that object, but I don't think that object is ever freed."

Yes, this is likely the case. I need to read up more on memory allocation and c...

iqbal-lab commented 9 years ago

We can talk about this. You just need a clear plan for ownership of memory - the person who allocates it shoudl free it, or handover to someone else who takes on responsibility for freeing it. I use valgrind to catch this stuff

iqbal-lab commented 9 years ago

This is still open. Valgrind output

==43917== 4 bytes in 1 blocks are definitely lost in loss record 1 of 58 ==43917== at 0x4A08320: malloc (vg_replace_malloc.c:263) ==43917== by 0x423D15: create_mask (base_species.c:275) ==43917== by 0x41BC48: create_non_aureus_mask (species.c:121) ==43917== by 0x41BC6F: non_aureus_panels_are_present (species.c:127) ==43917== by 0x41BDED: update_phylo_group_presence_and_coverage_from_species (species.c:176) ==43917== by 0x41BFF7: get_species_info (species.c:224) ==43917== by 0x403BAE: main (main.c:289) ==43917== ==43917== 16 bytes in 1 blocks are definitely lost in loss record 2 of 58 ==43917== at 0x4A06D70: calloc (vg_replace_malloc.c:566) ==43917== by 0x41875A: alloc_allele_info (genotyping_known.c:18) ==43917== by 0x4235F7: get_coverage_on_panels (base_species.c:27) ==43917== by 0x423C20: get_coverage_info (base_species.c:227) ==43917== by 0x41BF66: get_species_info (species.c:203) ==43917== by 0x403BAE: main (main.c:289) ==43917==

and here

==43917== 100 (24 direct, 76 indirect) bytes in 1 blocks are definitely lost in loss record 14 of 58 ==43917== at 0x4A08320: malloc (vg_replace_malloc.c:263) ==43917== by 0x428D6D: strbuf_create (in /Net/fs1/home/zam/dev/git/Mykrobe-predictor/bin/Mykrobe.predictor.staph) ==43917== by 0x41C1C8: load_all_cat_file_paths (species.c:284) ==43917== by 0x41BF80: get_species_info (species.c:210) ==43917== by 0x403BAE: main (main.c:289)

and here

==43917== 280 (24 direct, 256 indirect) bytes in 1 blocks are definitely lost in loss record 25 of 58 ==43917== at 0x4A08320: malloc (vg_replace_malloc.c:263) ==43917== by 0x428DEA: strbuf_new (in /Net/fs1/home/zam/dev/git/Mykrobe-predictor/bin/Mykrobe.predictor.staph) ==43917== by 0x423DE1: get_ith_phylo_group_name (base_species.c:308) ==43917== by 0x423E9F: print_json_indiv_phylo (base_species.c:331) ==43917== by 0x41C227: print_json_phylo_group (species.c:293) ==43917== by 0x423EE7: print_json_phylogenetics (base_species.c:337) ==43917== by 0x403C00: main (main.c:296)

and here

==43917== 480 bytes in 1 blocks are definitely lost in loss record 27 of 58 ==43917== at 0x4A08320: malloc (vg_replace_malloc.c:263) ==43917== by 0x418FBC: alloc_and_init_called_genes_array (genotyping_known.c:243) ==43917== by 0x403505: main (main.c:137)

and here

==43917== 2,464 bytes in 1 blocks are definitely lost in loss record 31 of 58 ==43917== at 0x4A08320: malloc (vg_replace_malloc.c:263) ==43917== by 0x41890B: alloc_and_init_called_variant_array (genotyping_known.c:108) ==43917== by 0x4034F4: main (main.c:136) ==43917==

Total lost memory - 360kb

iqbal-lab commented 9 years ago

These are easy to fix, but I want to talk it through

iqbal-lab commented 9 years ago

So create_mask mallocs an array that needs to be freed.

get_coverage_on_panels does this malloc

AlleleInfo* ai = alloc_allele_info();

but does not free it. Can just add free(ai) at the end of the function.

These type of functions load_all_phylo_group_file_paths

allocate string buffers. When you finish with your array of file paths, you need to free all of them.

Here:

char* get_ith_phylo_group_name(CovgInfo* covg_info, int i) { PhyloGroup phylo_group; StrBuf* phylo_group_name = strbuf_new();

this allocates something and returns it from the function. So the function caller should then assume ownership of the memory.

But the caller does not grab the returned object

void print_json_phylo_group(SpeciesInfo* species_info){ CovgInfo* covg_info =species_info->phylo_group_covg_info; int num_panels_present = covg_info->num_panels_present; print_json_phylo_group_start(); if (num_panels_present > 0){ print_json_indiv_phylo(covg_info,get_ith_phylo_group_name); }

looks like it passes the function down to be used elsewhere by someone who probably doesn't know they need to free the returned object.

iqbal-lab commented 9 years ago

Is this open?