#101 memory errors in adl.cpp

[waker]

i've encountered some heap corruption / crash bugs in adl.cpp module

1. for(int i = 199; i >= 0; i--) // this needs to be changed to 120
if(_trackEntries[i] != 0xff) {
numsubsongs = i + 1;
break;
}

2. void CadlPlayer::play(uint8_t track) {
uint8 soundId = _trackEntries[track];
if ((int8)soundId == -1 || !_soundDataPtr) // this check doesn't work for me, needs to be changed to soundId==0xff (without int8 cast)
return;

there are more memory errors, but i don't have fixes yet (valgrinding now. will send more info when i find out what's up)

bugs do not affect adplay, becayse it decodes/plays 1 tune at a time, but heavily impact stability of playlist based players, where all subtunes are scanned and added to playlist at once.

[waker]

might be related to bug #2994545

[waker]

i've fixed all valgrind complaints - and now it doesn't seem to crash or corrupt memory.
i'm sorry that i'm not providing proper direct patch against 2.2.1, but my fork of adplug is now way too different from upstream, so patching is very difficult.

stuff that needs to be looked at

1. getInstrument may read out of bounds. fix:

uint8 *getInstrument(int instrumentId) {
- return _soundData + READ_LE_UINT16(_soundData + 500 + 2 * instrumentId);
+ int offset = READ_LE_UINT16(_soundData + 500 + 2 * instrumentId);
+ if (offset == 0xffff) {
+ return NULL;
+ }
+ return _soundData + offset;
}

void AdlibDriver::setupInstrument(uint8 regOffset, uint8 *dataptr, Channel &channel) {
+ if (!dataptr) {
+ return;
+ }

2. max number of channels is 10, but the values are not validated, and sometimes much greater than that. easiest way to fix is as simple as that (and i'm not pretending this is the right fix)

- Channel _channels[10];
+ Channel _channels[0xff];

3. update method also needs to check for subsong validity

bool CadlPlayer::update()
{
+ uint8 soundId = _trackEntries[cursubsong];
+ if (soundId == 0xff || !_soundDataPtr) {
+ return false;
+ }
+ soundId &= 0xFF;
+ int offset = READ_LE_UINT16(_driver->_soundData + 2 * soundId);
+ if (offset == 0xffff) {
+ return false;
+ }

hope that helps

[waker]

and one more.. this fixed same kind of errors on other files. this whole ADL format support is a mess.
next time i will look into original scummvm ADL code, maybe it's possible to port newer version into adplug (in a hope they've fixed all this)

@@ -778,8 +778,19 @@ void AdlibDriver::executePrograms() {
opcode &= 0x7F;
if (opcode >= _parserOpcodeTableSize)
opcode = _parserOpcodeTableSize - 1;
debugC(9, kDebugLevelSound, "Calling opcode '%s' (%d) (channel: %d)", _parserOpcodeTable[opcode].name, opcode, _curChannel);
- result = (this->*(_parserOpcodeTable[opcode].function))(dataptr, channel, param);
+ if (opcode == 2) {
+ int offset = READ_LE_UINT16(_soundData + 2 * param);
+ if (offset == 0xffff) {
+ break; // corrupted file / bad parser
+ }
+ else {
+ result = (this->*(_parserOpcodeTable[opcode].function))(dataptr, channel, param);
+ }
+ }
+ else {
+ result = (this->*(_parserOpcodeTable[opcode].function))(dataptr, channel, param);
+ }