Bug fix the recent bam_plp_destroy memory leak removal #1754
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The pileup interface maintains a linked list of lbnode_t pointers. These start at iter->head, chain through lbnode->next, and then end up at iter->tail. We also have a separate linked list in iter->mp of items we've previously freed, so we don't have to free and malloc continually. bam_plp64_next adds and removes items to these linked lists, and it calls iter->plp_destruct when it puts items into the free list. So far so good, and so correct. However if for whatever reason we bail out of the pileup interface early, before we've punted all records onto the iter->mp free list, then we weren't calling plp_destruct on the current "in flight" data. This caused a memory leak, fixed in d028e0d. Unfortunately there is a subtlety I didn't notice at the time. The in-flight linked list goes from iter->head to *one before* iter->tail. The tail is simply a dummy node and unused by the code. I don't understand why it has to work this way, but presumably someone didn't want iter->head and iter->tail to ever point to the same item. The bam_plp_destroy function however has to move all these items to the iter->mp free list, so here it goes from iter->head to iter->tail inclusively. This commit avoids attempting to call the destructor on the tail, which could be a previously freed item that was pulled back off the iter->mp list, leading to double frees.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The pileup interface maintains a linked list of lbnode_t pointers. These start at iter->head, chain through lbnode->next, and then end up at iter->tail. We also have a separate linked list in iter->mp of items we've previously freed, so we don't have to free and malloc continually.
bam_plp64_next adds and removes items to these linked lists, and it calls iter->plp_destruct when it puts items into the free list. So far so good, and so correct.
However if for whatever reason we bail out of the pileup interface early, before we've punted all records onto the iter->mp free list, then we weren't calling plp_destruct on the current "in flight" data. This caused a memory leak, fixed in d028e0d.
Unfortunately there is a subtlety I didn't notice at the time. The in-flight linked list goes from iter->head to one before iter->tail. The tail is simply a dummy node and unused by the code. I don't understand why it has to work this way, but presumably someone didn't want iter->head and iter->tail to ever point to the same item.
The bam_plp_destroy function however has to move all these items to the iter->mp free list, so here it goes from iter->head to iter->tail inclusively. This commit avoids attempting to call the destructor on the tail, which could be a previously freed item that was pulled back off the iter->mp list, leading to double frees.