Some improvements for dmo.cpp#124
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.