search

tonioni / WinUAE

WinUAE Amiga emulator
http://www.winuae.net/
554 stars 90 forks source link

heads-up: possible int overflow in hsync_handler_post() (found & fixed in amiberry) #208

Closed nuumio closed 2 years ago

nuumio commented 2 years ago

Hi! I'm just giving a heads-up of possible int overflow in hsync_handler_post(): I experienced occasional freezes in Amiberry which originated from (int) casts in hsync_handler_post() function. See: https://github.com/midwan/amiberry/pull/880

Corresponding lines in WinUAE would be: https://github.com/tonioni/WinUAE/blob/master/custom.cpp#L11495 https://github.com/tonioni/WinUAE/blob/master/custom.cpp#L11520 (to line 11524) https://github.com/tonioni/WinUAE/blob/master/custom.cpp#L11560 https://github.com/tonioni/WinUAE/blob/master/custom.cpp#L11572

The problem in Amiberry was that vsyncmintime and rpt (and read_processor_time()) were unsigned longs (64-bit) and they were cast to ints (32-bit).

Changing casts to (int64_t) fixed those freezes in Amiberry. I'm not sure if this is a problem with WinUAE since Windows data model seems to be LLP64 (long and int both 32-bit) vs Linux LP64 (int 32-bit, long 64-bit). But since the code in question is similar in both I though to give a heads-up just in case :)

tonioni commented 2 years ago

It is always 32-bit in WinUAE (uae_u32). It shouldn't be 64-bit, even under 64-bit OS unless all comparisons are fixed to have correct casts. Or better yet, remove those ugly casts and comparisons because 64-bit counter does not need to handle wrap around (which happens every few seconds if 32-bit). I'll probably update this after 4.9.1.

nuumio commented 2 years ago

Thanks for the information! I'll leave it for you to decide if to close this issue or keep it open to track the change. I'll try to keep an eye on this on Amiberry side if you make the change and it gets merged.

tonioni commented 2 years ago

Since few weeks ago, all cycle counters are 64-bit. No more annoying wrap arounds to handle.