utsaslab / RECIPE

RECIPE : high-performance, concurrent indexes for persistent memory (SOSP 2019)
Apache License 2.0
197 stars 46 forks source link

Fix memory leak in get(char *) and del(char *) in P-Masstree. #25

Closed penglb3 closed 1 month ago

penglb3 commented 2 months ago

In the char * version of get() and del(), a leafvalue variable is dynamically allocated by make_leaf() but not properly freed. This results in memory leak and hence infinite memory growth as one uses P-Masstree.

I'm not an expert of PMem management or indexing, but I recon this variable is for temporary use, unlike other key-value data. The solution is quite straight-forward: use unique_ptr so the variable is automatically destructed and its memory freed as the function exits.

There might be memory leaks in other index structures like this. I didn't have time to check all of them. Another enhancement that could be done is a proper dtor that frees all dynamic memory resources when the index is on DRAM.

SeKwonLee commented 2 months ago

Hi @penglb3 ,

Thank you for your contribution! I really appreciate you identified this issue and raised the PR to solve it.

Using unique_ptr seems to make it easy to solve the issue. But, don't we also need to define a custom deleter for the object allocated by the make_leaf function, which employs posix_memalign instead of new, and specify it as a template argument for unique_ptr?

Thanks, Sekwon

penglb3 commented 2 months ago

Of course, using a customized deleter is better since it strictly matches alloc and dealloc. It might look like this:

std::unique_ptr<leafvalue, void(*)(leafvalue*)> lv(make_leaf(...), [](leafvalue* lv){ free(lv); });

Meanwhile, IMPOV make_leaf act as a combination of ctor and overloaded leafvalue::operator new(). Considering that leafvalue is trivial, calling delete (done implicitly by unique_ptr's dtor) shouldn't make a difference from free().

Anyway, the final design decision is yours :).