search

schellingb / TinySoundFont

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

Fix conditions evaluating based on uninitialized memory #63

Closed ell1e closed 1 year ago

ell1e commented 2 years ago

This changes two conditionals to no longer occasionally evaluate in part based on uninitialized memory: only presetIndex is guaranteed to be set in some cases when it is -1, so that should be evaluated first to avoid hitting the other subconditions if not needed. While this couldn't possibly cause behavior or corruption issues as far as I can tell, it still might distract from legitimate errors in valgrind, libasan/sanitizer, etc., and generally just cause uncertainty for library users when it pops up.

schellingb commented 2 years ago

Maybe it would make more sense to add a v->playingChannel = -1 to tsf_note_on because I guess in theory someone could start some voices through that and then switch over to use the tsf_channel_* functions?

Or maybe the API documentation should mention not to mix usage of channel functions and the lower level note on/off functions. As long as only one of the two groups of functions is used this should be moot.

ell1e commented 2 years ago

Maybe it would make more sense to add a v->playingChannel = -1 to tsf_note_on

The only reason I didn't do it that way in this pull request is because that is in theory marginally less efficient, so I thought I'd go for the least invasive change since I suppose at the end of the day this is a bit of a "cosmetic problem," or such. But otherwise I don't see why it couldn't be solved that way, sure.

Edit: so do you want me to change it? I personally think it doesn't really matter how exactly this detail is solved, but I'm happy to do it according to what you say