tildearrow / furnace

a multi-system chiptune tracker compatible with DefleMask modules
GNU General Public License v2.0
2.65k stars 209 forks source link

1800 (turn drums mode off) crashes OPL3 #2061

Closed Dullstar closed 3 months ago

Dullstar commented 3 months ago

To reproduce: In any OPL3-with-drums module, place an 1800 effect anywhere, then attempt to play it back. Furnace will crash nearly immediately once the effect is encountered. OPL and OPL2 are not affected. This only occurs when using Nuked-OPL3, but I did some debugging which showed the issue is not inside Nuked-OPL3.

The cause appears to be a segfault in DivPlatformOPL::acquire_nuked on the line starting with oscBuf[melodicChans+2]->data... (line 241 in opl.cpp as of the time of writing)

I believe what's happening is that when the command gets processed, it queues the write to the chip to disable drums mode, but it updates DivPlatformOPL::melodicChans immediately. When processing the queued writes, the current state of the emulated chip is referenced, so until this change is actually written, the if (fm.rhy&0x20) branch will segfault due to an out of bounds access to oscBuf. Before 1800 is encountered, melodicChans = 15 thus melodicChans + 4 is a valid index into the oscBuf array, but once it's encountered, melodicChans = 18, so it crashes once Furnace tries to write to oscBuf[melodicChans+2]. This would additionally explain why OPL/OPL2 aren't affected: they don't have enough melodic channels to overflow the buffer.

Changing these lines from melodicChans + 1, melodicChans + 2, etc. to a hardcoded 16, 17, 18, 19 will prevent the crash, but on OPL/OPL2 these would have different values so I can't guarantee this is the correct fix.

tildearrow commented 3 months ago

Test using Git master (or latest artifact in Actions tab).

Dullstar commented 3 months ago

Can confirm the crash no longer happens.

I will note that during testing I encountered other potential bugs with the command but they seem to be separate and (based on OPL2 behavior) were not newly introduced by the fix; once I've done more experimentation I'll report them separately if I still believe them to be Furnace bugs and not just how the chip works.

tildearrow commented 3 months ago

Great!