Nuke (PD) crashes with HD Mode 7

Screwtapello commented 3 years ago

Steps to reproduce

Make sure the Fast PPU is enabled, and HD Mode 7 is set to 480p or higher
Load the game "Nuke (PD)", SHA256 ee9bb8fe7fb52c57b84d8d0969ae97ff9440367158ebb75c28e72173f54ff103
From the title screen, pick "New Game"
This is a simple Minesweeper clone. Use the D-pad to select a tile, and the Start button to reveal what's underneath it.
Press Start when the cursor is over a mine, to lose the game

Expected results

A Mode 7 roto-zoom effect displaying a "Game Over" message

Actual results

Crash!

Notes

The crash does not occur in accurate-PPU mode, or in fast-PPU mode with HD Mode 7 disabled (240p).

The crash occurs with the default build=performance compiler options. Building with build=debug prevents the crash from occurring normally, but building in debug mode with the clang Address Sanitizer mode reports:

==525823==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000004553f68 at pc 0x0000009834e2 bp 0x62d0005f76b0 sp 0x62d0005f76a8
READ of size 2 at 0x000004553f68 thread T0
    #0 0x9834e1 in SuperFamicom::PPUfast::Line::cacheMode7HD() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/ppu-fast/mode7hd.cpp:43:50
    #1 0x98251a in SuperFamicom::PPUfast::Line::flush() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/ppu-fast/line.cpp:6:27
    #2 0x8637d8 in SuperFamicom::CPU::scanline() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/cpu/timing.cpp:137:26
    #3 0x89d0f8 in nall::function<void ()>::member<SuperFamicom::CPU>::operator()() const /home/st/Code/bsnes-emu/bsnes/bsnes/../nall/function.hpp:65:49
    #4 0x6cf768 in nall::function<void ()>::operator()() const /home/st/Code/bsnes-emu/bsnes/bsnes/../nall/function.hpp:29:47
    #5 0x885e55 in SuperFamicom::PPUcounter::tickScanline() /home/st/Code/bsnes-emu/bsnes/bsnes/./sfc/ppu/counter/counter-inline.hpp:39:16
    #6 0x885e55 in SuperFamicom::PPUcounter::tick() /home/st/Code/bsnes-emu/bsnes/bsnes/./sfc/ppu/counter/counter-inline.hpp:6:5
    #7 0x885e55 in SuperFamicom::CPU::stepOnce() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/cpu/timing.cpp:13:3
    #8 0x885e55 in void SuperFamicom::CPU::step<4u, false>() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/cpu/timing.cpp:38:30
    #9 0x866870 in SuperFamicom::CPU::read(unsigned int) /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/cpu/memory.cpp:41:3
    #10 0xb1eaf0 in Processor::WDC65816::fetch() /home/st/Code/bsnes-emu/bsnes/bsnes/processor/wdc65816/memory.cpp:31:10
    #11 0xb1eaf0 in Processor::WDC65816::instruction() /home/st/Code/bsnes-emu/bsnes/bsnes/processor/wdc65816/instruction.cpp
    #12 0x8647a3 in SuperFamicom::CPU::main() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/cpu/cpu.cpp:37:39
    #13 0x864593 in SuperFamicom::CPU::Enter() /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/cpu/cpu.cpp:30:9
    #14 0x7ce3ff  /home/st/Code/bsnes-emu/bsnes/bsnes/../libco/amd64.c:110:3

0x000004553f68 is located 168728 bytes to the right of global variable 'SuperFamicom::ppufast' defined in 'sfc/ppu-fast/ppu.cpp:10:5' (0x1e5f540) of size 40679184
SUMMARY: AddressSanitizer: global-buffer-overflow /home/st/Code/bsnes-emu/bsnes/bsnes/sfc/ppu-fast/mode7hd.cpp:43:50 in SuperFamicom::PPUfast::Line::cacheMode7HD()
Shadow bytes around the buggy address:
  0x0000808a2790: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a27a0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a27b0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a27c0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a27d0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
=>0x0000808a27e0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9[f9]f9 f9
  0x0000808a27f0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a2800: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a2810: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a2820: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0000808a2830: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==525823==ABORTING

The line where the crash occurs is this one: https://github.com/bsnes-emu/bsnes/blob/f57657f27ddec337b1960c7ddaa1b23894bc00c3/bsnes/sfc/ppu-fast/mode7hd.cpp#L43

DerKoun commented 3 years ago

Unverified guess: https://github.com/bsnes-emu/bsnes/blob/f57657f27ddec337b1960c7ddaa1b23894bc00c3/bsnes/sfc/ppu-fast/ppu.hpp#L361-L364 Maybe the 4 arrays are to short for this game/scene. If so, changing 32 to 240 for each one would solve the issue.

Screwtapello commented 3 years ago

Why are the arrays fixed-length? Was it too expensive to dynamically allocate them?

Alcaro commented 3 years ago

Since struct ppufast is already 40 megabytes (40 679 184 bytes, of which 39 813 120 are the Pixel arrays in Line::above/below), boosting these arrays to 240 is an absolutely trivial cost (+3328 bytes). Let's do it.

DerKoun commented 3 years ago

I didn't think it's necessary (and wanted to keep the code simple for my C++ inexperience). TBH I still don't. I mean the arrays only exist once per emulator instance......... aaaaaaaaaaand Alcaro just covered the numbers, so I'm done 😁

bsnes-emu / bsnes