Prevent infinite loop in envelope?

[bbbradsmith]

Suggestion that this...

https://github.com/BleuBleu/FamiStudio/blob/master/SoundEngine/famistudio_ca65.s#L2271

@env_special:
    bne @env_set_repeat  ; Zero is the loop point, non-zero values used for the repeat counter
    iny
    lda (@env_ptr),y     ; Read loop position
    tay
    jmp @env_read_value

Could have a check for an infinite loop:

@env_special:
    bne @env_set_repeat  ; Zero is the loop point, non-zero values used for the repeat counter
    sty famistudio_r1 ; remember the position of the loop command
    iny
    lda (@env_ptr),y     ; Read loop position
    tay
    cpy famistudio_r1 ; if we're looping back to the same loop command...
    beq @env_next_store_ptr ; infinite loop detected: terminate this envelope update
    jmp @env_read_value

This shouldn't happen under normal conditions, because the exporter shouldn't be capable of producing incorrect envelopes. However, in a game situation there are lots of ways things could fail... a music value could be set wrong by accident, etc. and a sound engine that can still manage to keep running on bad data (making funny noises) is a lot better than one that crashes the game entirely with an infinite loop.

In this example, the most likely cases where it comes up is if the envelope ends up pointed at 0s (this is how I discovered it). Two 0s at the start of an envelope is apparently the signal for an infinite loop, and in the current implementation it will hang forever.

Assumed famistudio_r1 was safe as a temporary here because it seems to get overwritten without read in the pitch update that follows the envelopes.

[BleuBleu]

That's a good idea. I can imagine many situation (dangling pointers, bank switching, etc.) that could lead to this.

Ill definitely add this for the next release. I will likely hide it behind a FAMISTUDIO_CFG_XXX, since there are also many cases where this isnt needed, such as the NSF / ROM / FDS export of famistudio, or smaller games than the one you are working on where the data is always loaded/available.

Unfortunately, 2.4.0 is all packaged and ready to go in the next couple of days, ill bundle that up in a bugfix release or the next one.

Thanks!

-Mat

[bbbradsmith]

It occurs to me that this probably is extremely unlikely to happen for anything but the case of 0s. It's very common to find strings of zeroes in a ROM, but the chances of a 0 (loop command) followed by a byte that exactly matches Y + also a bad pointer situation at the same time are probably insignificant.

So, instead of the check I added, if the loop command was simply some other value other than 0, it would solve this infinite loop potential with less overhead?