search

dmdedup / dmdedup3.19

Device-mapper Deduplication Target
20 stars 11 forks source link

Possible memory leak #25

Closed venkrishr closed 7 years ago

venkrishr commented 7 years ago

As we already discussed during our meeting, there is a possible memory leak in the following line:

https://github.com/dmdedup/dmdedup/blob/master/dm-dedup-rw.c#L213

I'm creating this issue because I'm not very confident about how to address this issue properly. If merge_data() fails, I am thinking of doing the following:

free_pages((unsigned long) page_address(page), 0) -> this is for the page created in create_bio() bio_put(clone) -> to destroy the clone clone = NULL

Is there anything else that I need to do? Or this would sufficiently fix the memory leak? Please advise.

sectorsize512 commented 7 years ago

To be honest, it seems like mege_data() can fail only if (!page || !bio_page(bio)). So, it might be easier to change merge_data() to return void and perform the above checks in the caller. What do you think?

venkrishr commented 7 years ago

Yes that would be a better idea.

If the condition passes, is it enough to do the 3 steps that I mentioned above?

sectorsize512 commented 7 years ago

I don't think those steps even necessary then. You can check that bio is not zero before you allocate the clone, then you don't need to deallocate the clone in case of an error. Also, clone creation via create_bio() fails if page is not allocated, therefore bio_page() cannot be NULL.

venkrishr commented 7 years ago

Is it necessary to check if bio is NOT NULL? The bio cannot possibly be NULL since it is the same pointer that gets passed from dm-dedup-target.c where we access it before even passing it.

sectorsize512 commented 7 years ago

Alright, Venkat, I don't think I need to consult you on every line of the code :) Just go ahead and make the changes that makes sense to you, don't be shy. Submit the patch and I review it. I'm sure, in most of the cases you will take the right decisions :)

sectorsize512 commented 7 years ago

Fixed with commit 6f950b96dca62fa2644dceeb23f68bf07f03b19c