milkytracker / MilkyTracker

An FT2 compatible music tracker
http://milkytracker.github.io/
Other
1.67k stars 160 forks source link

fix some UB #319

Closed nekopsykose closed 2 months ago

nekopsykose commented 11 months ago

initialise ticker to 0

on the first run, ticker points to uninit memory. the left shift in TimeRecord constructor by 8 makes it overflow past the left which is UB.

(i'm not 100% sure about this, but it makes ubsan happy)


i was hunting down a crash on start of the latest release, and found it to be fixed by https://github.com/milkytracker/MilkyTracker/commit/7e9171488fc47ad2de646a4536794fda21e7303d , after i found it myself too.

in the process i found more easy UB via ubsan, so i fixed that too.

for reference:

ticker 0:

(lldb) bt
* thread #1, name = 'milkytracker', stop reason = Invalid shift base
  * frame #0: 0x0000555555ce48b0 milkytracker`__ubsan_on_report at ubsan_monitor.cpp:39:40
    frame #1: 0x0000555555cdf559 milkytracker`::~Diag() at ubsan_diag.cpp:352:29
    frame #2: 0x0000555555ce2215 milkytracker`::handleShiftOutOfBoundsImpl() at ubsan_handlers.cpp:0
    frame #3: 0x0000555555ce1d3e milkytracker`__ubsan_handle_shift_out_of_bounds at ubsan_handlers.cpp:370:3
    frame #4: 0x00005555565e7955 milkytracker`PlayerBase::TimeRecord::TimeRecord(this=0x00007fffe5d72320, pos=0, row=0, tempo=3200171710, speed=3200171710, mainVol=255, ticker=-1094795586) at PlayerBase.h:119:17
    frame #5: 0x00005555565e75c3 milkytracker`PlayerBase::updateTimeRecord(this=0x0000625000000100) at PlayerBase.h:138:20
    frame #6: 0x00005555565e6dd0 milkytracker`PlayerBase::reallocTimeRecord(this=0x0000625000000100) at PlayerBase.h:131:3
    frame #7: 0x00005555565e0c55 milkytracker`PlayerBase::PlayerBase(this=0x0000625000000100, frequency=44100) at PlayerBase.cpp:104:2
    frame #8: 0x000055555660013c milkytracker`PlayerSTD::PlayerSTD(this=0x0000625000000100, frequency=44100, statusEventListener=0x0000627000000100) at PlayerSTD.cpp:215:2
    frame #9: 0x0000555555fc420a milkytracker`PlayerController::PlayerController(this=0x0000618000000480, mixer=0x0000608000001020, fakeScopes=false) at PlayerController.cpp:303:15
    frame #10: 0x0000555555ff6b88 milkytracker`PlayerMaster::createPlayerController(this=0x00006130000003c0, fakeScopes=false) at PlayerMaster.cpp:205:43
    frame #11: 0x000055555635d775 milkytracker`TabManager::createPlayerController(this=0x00006030000036a0) at TabManager.cpp:94:61
    frame #12: 0x00005555563793cd milkytracker`Tracker::Tracker(this=0x0000615000000080) at Tracker.cpp:158:33
    frame #13: 0x00005555564feae8 milkytracker`initTracker(bpp=4294967295, orientation=ORIENTATION_NORMAL, swapRedBlue=false, noSplash=false) at SDL_Main.cpp:811:18
    frame #14: 0x000055555650226b milkytracker`main(argc=1, argv=0x00007fffffffdc38) at SDL_Main.cpp:969:2
    frame #15: 0x00007ffff7f82820 ld-musl-x86_64.so.1`libc_start_main_stage2(main=(milkytracker`main at SDL_Main.cpp:888), argc=1, argv=0x00007fffffffdc38) at __libc_start_main.c:95:2
    frame #16: 0x0000555555c25bb7 milkytracker`_start + 22

the fixed bug already:

(lldb) bt
* thread #1, name = 'milkytracker', stop reason = Missing return
  * frame #0: 0x0000555555ce48b0 milkytracker`__ubsan_on_report at ubsan_monitor.cpp:39:40
    frame #1: 0x0000555555cdf559 milkytracker`::~Diag() at ubsan_diag.cpp:352:29
    frame #2: 0x0000555555ce2761 milkytracker`::handleMissingReturnImpl() at ubsan_handlers.cpp:425:3
    frame #3: 0x0000555555ce265e milkytracker`__ubsan_handle_missing_return at ubsan_handlers.cpp:432:3
    frame #4: 0x0000555556b9c892 milkytracker`MidiReceiver::countPorts(this=0x0000604000088a90) at MidiReceiver_pthread.cpp:147:2
    frame #5: 0x0000555556b9bd27 milkytracker`MidiReceiver::MidiReceiver(this=0x0000604000088a90, midiEventHandler=0x00005555570682a0) at MidiReceiver_pthread.cpp:45:23
    frame #6: 0x00005555564f5a44 milkytracker`StartMidiRecording(devID=0) at SDL_Main.cpp:275:23
    frame #7: 0x00005555564f5b56 milkytracker`InitMidi() at SDL_Main.cpp:287:2
    frame #8: 0x00005555564ff65a milkytracker`initTracker(bpp=4294967295, orientation=ORIENTATION_NORMAL, swapRedBlue=false, noSplash=false) at SDL_Main.cpp:835:2
    frame #9: 0x000055555650226b milkytracker`main(argc=1, argv=0x00007fffffffdc38) at SDL_Main.cpp:969:2
    frame #10: 0x00007ffff7f82820 ld-musl-x86_64.so.1`libc_start_main_stage2(main=(milkytracker`main at SDL_Main.cpp:888), argc=1, argv=0x00007fffffffdc38) at __libc_start_main.c:95:2
    frame #11: 0x0000555555c25bb7 milkytracker`_start + 22

you should be able to reproduce the same with -fsanitize=undefined and a toolchain with sanitizers. there's probably a bunch more to fix..

coderofsalvation commented 10 months ago

thank you very much, I tried it and didn't see any side-effects. Did it crash on your system?