Some of NSFE files are can't be played now (In previous versions there are worked fine)

[Wohlstand]

Recently I have found that two of my NSFE files are no more working with GME! However, there are worked fine in previous versions.

The error message I getting is the Error: Corrupt file.

[Michael Pyne]

Thanks for the report. Having not reproduced yet I would suspect it's related to some of the security hardening measures I'd put in place after CVE-2017-17446.

[Wohlstand]

My system is Linux Mint 18.2, I using a custom-installed GCC 6.4, and system is 64-bit as build is also 64-bit. Also, at me sanitizer have produced some shared linking errors and lots of linking errors on attempt to link static build. Therefore I have disabled it by a CMake's flag. Maybe I need to try to build it as 32-bit? If that will work in 32-bits, then, you have some cross-architecture mistake related on a type sizes.

[Wohlstand]

Okay, just now I have built the stuff on my very old computer but on modern Linux Mint with XFCE environment, I have used G++ 5.4 and built with default CMake config cmake ... Inability to play those files is still reproducable:

[Wohlstand]

The full build log where are also two warnings have been found:

vitaly@whl-pc-002l ~/_git_repos/game-music-emu $ mkdir build
vitaly@whl-pc-002l ~/_git_repos/game-music-emu $ cd build/
vitaly@whl-pc-002l ~/_git_repos/game-music-emu/build $ cmake ..
-- The C compiler identification is GNU 5.4.0
-- The CXX compiler identification is GNU 5.4.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test __LIBGME_SWITCH_FALLTHROUGH_WARNINGS
-- Performing Test __LIBGME_SWITCH_FALLTHROUGH_WARNINGS - Failed
 ** ZLib library located, compressed file formats will be supported
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - not found
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE  
SDL library not found, disabling player demo build
-- Configuring done
-- Generating done
-- Build files have been written to: /home/vitaly/_git_repos/game-music-emu/build
vitaly@whl-pc-002l ~/_git_repos/game-music-emu/build $ make
Scanning dependencies of target gme
[  2%] Building CXX object gme/CMakeFiles/gme.dir/Blip_Buffer.cpp.o
[  4%] Building CXX object gme/CMakeFiles/gme.dir/Classic_Emu.cpp.o
[  6%] Building CXX object gme/CMakeFiles/gme.dir/Data_Reader.cpp.o
/home/vitaly/_git_repos/game-music-emu/gme/Data_Reader.cpp: In member function ‘virtual long int Std_File_Reader::read_avail(void*, long int)’:
/home/vitaly/_git_repos/game-music-emu/gme/Data_Reader.cpp:375:27: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  if ( file_ && s > 0 && s <= UINT_MAX ) {
                           ^
In file included from /home/vitaly/_git_repos/game-music-emu/gme/Data_Reader.cpp:21:0:
/home/vitaly/_git_repos/game-music-emu/gme/Data_Reader.cpp: In member function ‘virtual const char* Std_File_Reader::read(void*, long int)’:
/home/vitaly/_git_repos/game-music-emu/gme/Data_Reader.cpp:389:36: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  RETURN_VALIDITY_CHECK( s > 0 && s <= UINT_MAX );
                                    ^
/home/vitaly/_git_repos/game-music-emu/gme/blargg_source.h:28:44: note: in definition of macro ‘unlikely’
     #define unlikely( x ) __builtin_expect(x, 0)
                                            ^
/home/vitaly/_git_repos/game-music-emu/gme/Data_Reader.cpp:389:2: note: in expansion of macro ‘RETURN_VALIDITY_CHECK’
  RETURN_VALIDITY_CHECK( s > 0 && s <= UINT_MAX );
  ^
[  8%] Building CXX object gme/CMakeFiles/gme.dir/Dual_Resampler.cpp.o
[ 10%] Building CXX object gme/CMakeFiles/gme.dir/Effects_Buffer.cpp.o
[ 12%] Building CXX object gme/CMakeFiles/gme.dir/Fir_Resampler.cpp.o
[ 14%] Building CXX object gme/CMakeFiles/gme.dir/gme.cpp.o
[ 17%] Building CXX object gme/CMakeFiles/gme.dir/Gme_File.cpp.o
[ 19%] Building CXX object gme/CMakeFiles/gme.dir/M3u_Playlist.cpp.o
[ 21%] Building CXX object gme/CMakeFiles/gme.dir/Multi_Buffer.cpp.o
[ 23%] Building CXX object gme/CMakeFiles/gme.dir/Music_Emu.cpp.o
[ 25%] Building CXX object gme/CMakeFiles/gme.dir/Ay_Apu.cpp.o
[ 27%] Building CXX object gme/CMakeFiles/gme.dir/Ym2612_Emu.cpp.o
[ 29%] Building CXX object gme/CMakeFiles/gme.dir/Sms_Apu.cpp.o
[ 31%] Building CXX object gme/CMakeFiles/gme.dir/Ay_Cpu.cpp.o
[ 34%] Building CXX object gme/CMakeFiles/gme.dir/Ay_Emu.cpp.o
[ 36%] Building CXX object gme/CMakeFiles/gme.dir/Gb_Apu.cpp.o
[ 38%] Building CXX object gme/CMakeFiles/gme.dir/Gb_Cpu.cpp.o
[ 40%] Building CXX object gme/CMakeFiles/gme.dir/Gb_Oscs.cpp.o
[ 42%] Building CXX object gme/CMakeFiles/gme.dir/Gbs_Emu.cpp.o
[ 44%] Building CXX object gme/CMakeFiles/gme.dir/Gym_Emu.cpp.o
[ 46%] Building CXX object gme/CMakeFiles/gme.dir/Hes_Apu.cpp.o
[ 48%] Building CXX object gme/CMakeFiles/gme.dir/Hes_Cpu.cpp.o
[ 51%] Building CXX object gme/CMakeFiles/gme.dir/Hes_Emu.cpp.o
[ 53%] Building CXX object gme/CMakeFiles/gme.dir/Kss_Cpu.cpp.o
[ 55%] Building CXX object gme/CMakeFiles/gme.dir/Kss_Emu.cpp.o
[ 57%] Building CXX object gme/CMakeFiles/gme.dir/Kss_Scc_Apu.cpp.o
[ 59%] Building CXX object gme/CMakeFiles/gme.dir/Nes_Apu.cpp.o
[ 61%] Building CXX object gme/CMakeFiles/gme.dir/Nes_Cpu.cpp.o
[ 63%] Building CXX object gme/CMakeFiles/gme.dir/Nes_Fme7_Apu.cpp.o
[ 65%] Building CXX object gme/CMakeFiles/gme.dir/Nes_Namco_Apu.cpp.o
[ 68%] Building CXX object gme/CMakeFiles/gme.dir/Nes_Oscs.cpp.o
[ 70%] Building CXX object gme/CMakeFiles/gme.dir/Nes_Vrc6_Apu.cpp.o
[ 72%] Building CXX object gme/CMakeFiles/gme.dir/Nsf_Emu.cpp.o
[ 74%] Building CXX object gme/CMakeFiles/gme.dir/Nsfe_Emu.cpp.o
[ 76%] Building CXX object gme/CMakeFiles/gme.dir/Sap_Apu.cpp.o
[ 78%] Building CXX object gme/CMakeFiles/gme.dir/Sap_Cpu.cpp.o
[ 80%] Building CXX object gme/CMakeFiles/gme.dir/Sap_Emu.cpp.o
[ 82%] Building CXX object gme/CMakeFiles/gme.dir/Snes_Spc.cpp.o
[ 85%] Building CXX object gme/CMakeFiles/gme.dir/Spc_Cpu.cpp.o
[ 87%] Building CXX object gme/CMakeFiles/gme.dir/Spc_Dsp.cpp.o
[ 89%] Building CXX object gme/CMakeFiles/gme.dir/Spc_Emu.cpp.o
[ 91%] Building CXX object gme/CMakeFiles/gme.dir/Spc_Filter.cpp.o
[ 93%] Building CXX object gme/CMakeFiles/gme.dir/Vgm_Emu.cpp.o
[ 95%] Building CXX object gme/CMakeFiles/gme.dir/Vgm_Emu_Impl.cpp.o
[ 97%] Building CXX object gme/CMakeFiles/gme.dir/Ym2413_Emu.cpp.o
[100%] Linking CXX shared library libgme.so
[100%] Built target gme

[Michael Pyne]

I've reproduced the corrupt file message with current git. I'll see if I can bisect to the relevant commit.

As for the sanitizer flags, I believe those are only supported with shared libs anyways so I probably need to disable one or the other in CMake if both static libs and sanitizer is requested, or abort the build with a message saying to pick one or the other.

[Wohlstand]

About of sanitizer, the corruption is reproducible with no matter to sanitizer flag state, and with no matter by which I built GME: by original CMake script, by my custom hand-make simplified CMake or by QMake I also made: https://github.com/WohlSoft/AudioCodecs/tree/master/libgme It's probably a mistake in a file reader or in the code that parses NSFE files. And, with no matter static or dynamic build.

[Michael Pyne]

Seems to be commit 205290614cdc057541b26adeea05a9d45993f860, which was applied back in Dec. 2017 to fix a crash and potential out-of-bounds memory access (see Issue #14). That's what git-bisect found and I can confirm that reverting that commit allows the player to operate.

I'm not sure if this means the a2xt-wandering.nsfe file is actually corrupt or whether we need to go and improve that sanity check as I'd mentioned in that commit though.

[Wohlstand]

Let's check the NSFE specification and the actual hex data in both those files, there are both existing as-is at 3 years... I bet, when the size is given negative, needed some different logic, OR, that size must be an unsigned value...

Found this: http://wiki.nesdev.com/w/index.php/NSFe And yeah, that value must be unsigned

[Michael Pyne]

I think I found a workable fix for now, if I relax the check to only fail on negative block sizes then the files you submit play back fine while the issue 14 corrupt file is still rejected. I'm prepping the commit now.

[Michael Pyne]

nsfe: Some blocks can have a 0-sized header, don't fail for those.

This fixes a regression introduced by commit 205290614cdc057541b26adeea05a9d45993f860, where we added a check for potentially malicious NSFE file dumps to avoid crashes or potential OOB mem access.

It seems some valid files can have 0-sized blocks, so let those through the first check. This allows the testcases for Issue #22 to pass while still rejecting the corrupt file on Issue #14.

Fixes Issue #22.

→ <<cset 63119537125e>>

[Wohlstand]

Neat! Gotta to check out the result...

[Wohlstand]

Everything works at me! Thanks!

[Michael Pyne]

Great to hear, thanks for raising the issue!