schellingb / TinySoundFont

SoundFont2 synthesizer library in a single C/C++ file
MIT License
608 stars 71 forks source link

`TSF_MALLOC(...)` without non-NULL check/error handling #61

Closed ell1e closed 2 years ago

ell1e commented 2 years ago

I've checked the code, and TSF_MALLOC(...) appears to be used in many places without non-NULL check/error handling. Since such "out of memory unawareness" in code always infects each entire program that uses this library, and it's not a gigantic amount of work to do better, it would be nice if this could be fixed. Basically, any use of TSF_MALLOC should be followed by a check if the allocated item is not NULL, and if it is NULL, resulting safe error bail-out code that doesn't crash. Here are some (not all!) of the problematic spots:

https://github.com/schellingb/TinySoundFont/blob/bf574519e601202c3a9d27a74f345921277eed39/tsf.h#L731

https://github.com/schellingb/TinySoundFont/blob/bf574519e601202c3a9d27a74f345921277eed39/tsf.h#L830

https://github.com/schellingb/TinySoundFont/blob/bf574519e601202c3a9d27a74f345921277eed39/tsf.h#L1223

...and so on.

ell1e commented 2 years ago

Related: I discovered tsf_set_max_voices, despite having a doc comment saying it does allocation, returns void. It should return int with the return value being e.g. 1 on success, 0 on allocation failure.

schellingb commented 2 years ago

Hey there. Now that there is plenty of usage of this library in embedded systems and other platforms with small amounts of memory this certainly is warranted. I wish I had thought of this when originally writing the code but I was in my usual mood of targeting the higher end platforms with fancy virtual memory and swapping. It looks like tsf_channel_init might be the most annoying one to fix? Maybe it can be split up in fixing the allocation of non-channel API first. Most allocations are during loading which can just return NULL. New voices would just not play if the realloc there fails which should be fine, too. Would you be interested in trying to improve things here? Otherwise I can see if I can find a few hours to do this.

ell1e commented 2 years ago

Yes, I'd be interested in investing some hours soon to try a pull request for this. (No promises that I'll manage to catch all instances but I'll try, and I'd only do tsf.h and not tml.h since I happen to use a different parser). However, it might make sense to test & merge #1 first to avoid conflicts.

Edit: I made #65 now as a rebased #1 to speed things up, hopefully