Skip to content

Some improvements for dmo.cpp#124

Merged
miller-alex merged 7 commits into
adplug:masterfrom
miller-alex:dmo
Nov 25, 2020
Merged

Some improvements for dmo.cpp#124
miller-alex merged 7 commits into
adplug:masterfrom
miller-alex:dmo

Conversation

@miller-alex

Copy link
Copy Markdown
Contributor

I'm using a PR for this batch because I had most of it lying around unfinished for some months, maybe I missed something.
Also, since I'm re-enabling the TwinTeam sample in playertest, you may want to test whether the inconsistent test results are really fixed before committing to master.

The test passes consistently in my setup, and there doesn't seem to
be anything that's supposed to produce varying results in the player.
The inconsistent results reported earlier may have been caused by code
exhibiting undefined behaviour that has been fixed in the meantime
(perhaps commit 39d0799, "Fix undefined behaviour"). If the test
still fails in some setups that's a bug that should be fixed instead
of hiding it.

This commit re-enables the TwinTrack sample in playertest, reverting
commit 95310fd ("Disable TwinTrack tests"), and removes it from
stresstest, where it was initially added as an example.
File data decryption verifies a 16 bit checksum and the unpacked
data contains a 22 byte signature. That's more than enough to
identify the file format without relying on the extension.
There were no checks for the size of the input data in decrypt()
and unpack(). Add them, along with the following changes:
* Add inputsize parameter to unpack() (needed for the checks).
* Pass whole input data to unpack() (including decryption header).
* Pass output length to unpack_block(), drop oend from the class
  and make unpack() and unpack_block() static methods.
* Use size_t where appropriate.
* Add an enum constant headersize for the size of the decryption
  header.
* Clean up the unpack code a little bit.
* Remove now unused CHARP_AS_WORD macro.
* Set last byte of song name and instrument names to NUL to
  ensure string termination.

* Refuse to load file when order/insruments/pattern number in
  the header is too big to avoid writing past the end of the
  arrays while loading data. (Note that we can use only 255 orders
  because of "orders[header.ordnum] = 0xFF;". If there exist
  files with 256 orders that needs to change.)

The loaded data is not sanity checked, crashes during playback
can still happen. But in order to know what needs to be checked,
Cs3mPlayer has to be fixed first.
@miller-alex miller-alex merged commit e88b7c8 into adplug:master Nov 25, 2020
@miller-alex miller-alex deleted the dmo branch November 25, 2020 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant