TinySoundFont icon indicating copy to clipboard operation
TinySoundFont copied to clipboard

Fix conditions evaluating based on uninitialized memory

Open ell1e opened this issue 4 years ago • 2 comments
trafficstars

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.

ell1e avatar Nov 02 '21 22:11 ell1e

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.

schellingb avatar Nov 05 '21 16:11 schellingb

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

ell1e avatar Nov 05 '21 18:11 ell1e