08952: spec128: Several tapes cause MAME to crash

ID	Category [?]	Severity [?]	Reproducibility	Date Submitted	Last Update
08952	Crash/Freeze	Critical (emulator)	Always	Nov 5, 2024, 14:53	16 days ago

Tester	Robbbert	View Status	Public	Platform	MAME (Self-compiled)
Assigned To		Resolution	Open	OS	Windows 10/11 (64-bit)
Status [?]	Confirmed			Driver	sinclair/spec128.cpp
Version	0.271	Fixed in Version		Build	64-bit
		Fixed in Git Commit		Github Pull Request #	#13294

Summary	08952: spec128: Several tapes cause MAME to crash
Description	While testing my loose software, it was noted that several tapes cause MAME to crash as soon as the emulation is started.
Steps To Reproduce	Enter this line, using the supplied file, and substituting your path. mame spec128 -cass "e:\data\sinclair\spectrum\Automaticky Bubenik Verze 2 (1986)(Daniel Rodny).tap" It will immediately crash, before the screen can appear.
Additional Information	I do not know if these tapes are meant for this system, however even if that's the case, a crash should not occur. A number of examples have been included. Although I didn't test it, I'd imagine that most of the spectrum-related systems will crash in the same way. C:\MAME>mame spec128 -cass "e:\data\sinclair\spectrum\Automaticky Bubenik Verze 2 (1986)(Daniel Rodny).tap" Warning: layout view 'Keyboard Layout' contains deprecated cpanel element Warning: layout view 'Keyboard Only' contains deprecated cpanel element Warning: layout view 'Keyboard Layout' contains deprecated cpanel element Warning: layout view 'Keyboard Only' contains deprecated cpanel element ----------------------------------------------------- Exception at EIP=00007ff7cb28a690 (tzx_cas_handle_block(short*, unsigned char const, int, int, int, int, int, int, int, int, int) [clone .constprop.0]+0x0150): ACCESS VIOLATION While attempting to read memory at 0000029b8b19b000 ----------------------------------------------------- RAX=0000000000000000 RBX=0000000000005a9e RCX=0000000000005a9e RDX=0000000000000000 RSI=0000000000003d00 RDI=0000029b8b197300 RBP=00000000000003e8 RSP=000000f43e6f8c00 R8=0000000000000016 R9=00000000002b5754 R10=0000000000000001 R11=000000000000000b R12=0000000000005b02 R13=0000000000000008 R14=0000000000005b01 R15=0000000000004ca3 ----------------------------------------------------- Stack crawl: 000000f43e6f8c30: 00007ff7cb28a690 (tzx_cas_handle_block(short*, unsigned char const, int, int, int, int, int, int, int, int, int) [clone .constprop.0]+0x0150) 000000f43e6f8cb0: 00007ff7cb28a7bb (tap_cas_to_wav_size(unsigned char const, int)+0x003b) 000000f43e6f8e30: 00007ff7cdea40db (cassette_image::legacy_construct(cassette_image::LegacyWaveFiller const)+0x024b) 000000f43e6f8ec0: 00007ff7cdea193c (cassette_image::open_choices(std::unique_ptr<util::random_read_write, std::default_delete<util::random_read_write> >&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cassette_image::Format const* const, int, std::unique_ptr<cassette_image, std::default_delete<cassette_image> >&)+0x010c) 000000f43e6f8fd0: 00007ff7cac69f22 (cassette_image_device::internal_load(bool)+0x00c2) 000000f43e6f9040: 00007ff7cac6a5e5 (non-virtual thunk to cassette_image_device::call_load[abi:cxx11]()+0x0035) 000000f43e6f90f0: 00007ff7caca363e (device_image_interface::finish_load[abi:cxx11]()+0x026e) 000000f43e6f9210: 00007ff7cdc9b6fb (image_manager::postdevice_init()+0x017b) 000000f43e6f9240: 00007ff7d50950c2 (luaopen_lfs+0x2709222) 000000f43e6f9380: 00007ff7ca9a3dc8 (device_t::start()+0x0698) 000000f43e6f94d0: 00007ff7cab36d9a (running_machine::start_all_devices()+0x014a) 000000f43e6f95f0: 00007ff7cab3ae31 (running_machine::start()+0x0a91) 000000f43e6f9770: 00007ff7cab3e3dc (running_machine::run(bool)+0x00cc) 000000f43e6fed90: 00007ff7cdc6d15c (mame_machine_manager::execute()+0x024c) 000000f43e6ff180: 00007ff7d195d49a (cli_frontend::start_execution(mame_machine_manager, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&)+0x03ea) 000000f43e6ff490: 00007ff7d195daca (cli_frontend::execute(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&)+0x007a) 000000f43e6ff4f0: 00007ff7cdc67f07 (emulator_info::start_frontend(emu_options&, osd_interface&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&)+0x0027) 000000f43e6ff8c0: 00007ff7d35e6141 (luaopen_lfs+0xc5a2a1) 000000f43e6ff910: 00007ff7c6de12ee (__tmainCRTStartup+0x016e) 000000f43e6ff940: 00007ff7c6de1406 (mainCRTStartup+0x0016) 000000f43e6ff970: 00007ffd2c997374 (BaseThreadInitThunk+0x0014) 000000f43e6ff9f0: 00007ffd2cadcc91 (RtlUserThreadStart+0x0021)
Github Commit

Flags
Regression Version
Affected Sets / Systems	spec128

Attached Files	Spec128 crashers.zip (298,745 bytes) Nov 5, 2024, 14:53 Uploaded by Robbbert

No.22410 Robbbert Moderator Nov 5, 2024, 14:54	I've barely begun the testing of spectrum tapes, so I'd expect there will be many more crashes encountered.
No.22438 Robbbert Moderator Nov 13, 2024, 22:50	I did a little investigation without finding a definite answer. It does look like a buffer overflow for the variable "buffer", but I was not able to find where this variable is declared or how much memory is allocated to it.
No.22517 holub Tester Dec 3, 2024, 15:04 edited on: Dec 3, 2024, 15:08	The overflow caused by invalid wav samples buffer calculations. https://github.com/mamedev/mame/blob/master/src/lib/formats/cassimg.cpp#L849 Changing this to `samples.resize(sample_count * 2);` allows tapes to load but as value is not precise, UI keep showing tape loading after it finished.
No.22917 holub Tester 24 days ago	All reported images are wrong. In most cases they requesting more data in the header than available in the image file. Adding guards allows to load few of them, so considering this as a better solution than error: https://github.com/mamedev/mame/pull/13289
No.22918 JimCarlTay Tester 24 days ago	Pull request #13289 was merged. Has it managed to fix this bug?
No.22919 holub Tester 24 days ago	#13289 is meeged but just reverted with #13292 because we found degradation in other cassetes caused by it. I have and idea how to bring it back, just need some time to sort some things up
No.22920 holub Tester 24 days ago	"fix the bug" - not really. #13289 prevents exception, but not so much we can do with wrong images.
No.22921 holub Tester 23 days ago	I hope #13294 is final solution
No.22926 Robbbert Moderator 20 days ago edited on: 20 days ago	Still getting crashes with latest git. Actually, all tap files are crashing, so it's a regression. I'll make a new report for it.
No.22930 Robbbert Moderator 18 days ago edited on: 18 days ago	Even after MT09104, still getting some crashes happening with TAP files. Try Alien Storm which is attached.
No.22931 holub Tester 18 days ago edited on: 18 days ago	These attachments required #13294 which Vas doesn't like or still don't have time to because of arm64
No.22939 Haze Senior Tester 17 days ago	is there a reason to not just report that the tape file is corrupt and should be replaced? MAME typically doesn't go out of its way to support known bad media
No.22945 holub Tester 16 days ago edited on: 15 days ago	That's the possibility. My concern is that some of them have correct data but wrong header which allows to load anyway. That's why I suggested UI warning in the PR but have no conclusion on this yet.