jackaudio / jack1

jack1 codebase
Other
250 stars 70 forks source link

Fixed leaked memory with jack_tmpdir. #47

Closed Uladox closed 8 years ago

Uladox commented 8 years ago

When new clients are created and jack_get_tmpdir() is called, jack_tmpdir is not free'd resulting in memory being leaked since it is set to a new value from malloc and unable to be free'd later. This means that if you are writing a program that creates and deletes clients regularly there will be a serious memory issue. Also, even when all clients are closed, the malloc'd jack_tmpdir is not free'd which makes it impossible to free all allocated memory by the end of a program's execution making detecting real memory leaks a bit more difficult. This commit fixes this by keeping track of how many clients there are and acting on memory based off such. Basically with this and VALGRIND_MEMSET defined, valgrind will no longer complain! 😄

jdek commented 8 years ago

This just seems like a really dirty fix, introducing another global variable. The solution here would be to find a way to not need to use the tmpdir global at all.

Uladox commented 8 years ago

I felt that as well, but since tmpdir was used as a global variable I wasn't quite sure what to do. One of the things that bothered me was how it was set in client_open_aux, but used in internal clients, even though internal clients never called the jack_get_tmpdir function which sets the value. If it could not be used in jack_user_dir and other files it is exposed to by internal.h, then the variable itself could be removed entirely.

Uladox commented 8 years ago

Okay, I got rid of the dirty fix and tried (although I do not claim to know jack inside and out so I would be unsuprised if I did something wrong) a more proper approach by looking at everywhere that the global was used and addressing that. So now the global jack_tmpdir is gone and instead jack_get_tmpdir() is used (and is changed itself to be more general returning the tmpdir instead of changing gloabal state) in most of the places the global was used. There are some exceptions which are appressed in the "Removes use of gloabl jack_tmpdir" commit, but all of them result in the exact same behavior as before so I hope they do not cause any issues. If there are any critcisms you have I would be happy to hear, I have a better understanding of the project now and really want to get rid of those valgrind messages. :bowtie:

jdek commented 8 years ago

This looks mostly fine, now you just have to wait until it gets merged. (Which could be a long time).