grimfang4 / SDL_FontCache

A generic font caching C library with loading and rendering support for SDL.
MIT License
305 stars 66 forks source link

Fixed a memory leak that occured on FC_Init #21

Closed MonzUn closed 6 years ago

MonzUn commented 6 years ago

FC_GetStringASCII does malloc and returns the pointer so directly passing it to U8_strdup will cause a memory leak.

I'm not sure why we there is a need to do U8_strdup here though so I just craeted a temp pointer so that it can be freed after the copy instead of just removing U8_strdup.

grimfang4 commented 6 years ago

I think this was from an older implementation that used string literals. That U8_strdup() shouldn't be there. Also, glancing over it, FC_GetStringASCII_Latin1() leaks twice the same way. If you remove the U8_strdup() I'll merge it.

MonzUn commented 6 years ago

Sounds good; although I just realized I broke const when doing the free() though(I'm used to C++ and there it would have caused a compilation error). How should we handle that? font->loading_string = FC_GetStringASCII(); will have the same problem.

I guess you could const cast on the free only but it seems the loading_string needs to be non-const in FC_LoadFontFromTTF() as well.

Edit: Never mind on that last part.

MonzUn commented 6 years ago

Should I also remove const from FC_GetStringASCII()? FC_SetLoadingString() requires loading_string to be non const and if it is just const casted onFC_GetStringASCII() there not really a point to it being const in the first place...?

Excuse my uncertainty but I have very lite knowledge about how FC actually works; I just found the memory leak when debugging my own code :)

MonzUn commented 6 years ago

If the U8_strdup() is removed FC_SetLoadingString will free the static pointer in FC_GetStringASCII() and the next time FC_GetStringASCII() it would just return a pointer to freed memory :/

This wasn't as trivial as I expected^^'

grimfang4 commented 6 years ago

Yeah, none of these should have const: const char FC_GetStringASCII(void); const char FC_GetStringLatin1(void); const char* FC_GetStringASCII_Latin1(void);

grimfang4 commented 6 years ago

The way to fix the static buffer thing is to return U8_strdup(buffer) from those functions. There's still technically a leak with those static buffers since there's no function to clean up FC's internal state. I don't mind either way if that approach is kept or the strings are recalculated every time.

MonzUn commented 6 years ago

Sure; that would cause a leak of static char* buffer at shutdown instead though but that's harmless. Are you fine with that?

grimfang4 commented 6 years ago

Yep, that's fine.

MonzUn commented 6 years ago

Should be fixed now. I squished the previous commit since i didn't know if it would have been added to your history or not.

grimfang4 commented 6 years ago

I think I'll remove the static buffer thing eventually, too. This implementation was trying to be clever, but it's probably best for it to be simple and clear.

grimfang4 commented 6 years ago

Thanks for the attention to this! :D

MonzUn commented 6 years ago

Well; thanks for helping me do text rendering in SDL without a hassle or kind of bad performance issues^^

I was actually going to see if I could get rid of the memory leaks at shutdown (without affecting performance) as there is at least one more and I'd like to get my memory debug output clear so I notice when I actually create real memory leaks in my own code :P. I will look into it tomorrow or something and submit a new pull request if I'm happy with the results.