dirkwhoffmann / virtualc64

VirtualC64 is a cycle-accurate C64 emulator for macOS
https://dirkwhoffmann.github.io/virtualc64
Other
353 stars 33 forks source link

Crash during shutdown #798

Closed dirkwhoffmann closed 3 months ago

dirkwhoffmann commented 3 months ago

To my shame, I haven't managed to implement a safe shutdown procedure for years. It still crashes from time to time on exit (e.g., today):

Bildschirmfoto 2024-06-14 um 19 09 41
dirkwhoffmann commented 3 months ago

Hopefully fixed in v5.0b5

I've added a locking mechanism to the audio unit. My suspicion is that the audio unit callbacks are still invoked after the audio unit has been shut down.

mithrendal commented 3 months ago

I finally figured out how to reproduce the shutdown bug, it is still present in v5.0.beta6

how to reproduce

  1. open virtualc64
  2. goto preferences and set runahead to a value greater than 0
  3. click on the red button to close the window
  4. a dialog pops up "do you want to keep this new document "Untitled"?
  5. choose delete
  6. windows closes and macos crash reporter opens

it only crashes with runahead enabled (i.e. > 0), when runahead is disabled (i.e. ==0) shutdown procedure behaves normal 😎.

UPDATE: It is sufficient for the crash that runahead > 0 was active, i.e., even if it was disabled (set to 0) before the shutdown, it will still lead to a crash.

dirkwhoffmann commented 3 months ago

Confirmed! Thanks for making the bug reproducible!

Bildschirmfoto 2024-06-26 um 21 24 27

I'm already on his trail... stay tuned...

dirkwhoffmann commented 3 months ago

Here is what's happening: Class Keyboard has an overloaded = operator:

Keyboard& operator= (const Keyboard& other) {

        CLONE_ARRAY(kbMatrixRow)
        CLONE_ARRAY(kbMatrixCol)
        CLONE_ARRAY(kbMatrixRowCnt)
        CLONE_ARRAY(kbMatrixColCnt)
        CLONE(shiftLock)
        CLONE(pending)

        return *this;
    }

Variable pending is a RingBufferwhich has the following members:

template <class T, isize capacity> struct RingBuffer
{
    // Element storage
    T *elements = new T[capacity]();

    // Read and write pointers
    isize r, w;

    ...

The bug is that RingBuffer does not overload the = operator itself. The default implementation copies all members over which means that elements points to the same memory area after the assignment. When run-ahead is active, we end up with two emulator instances sharing the same elements array. The app crashes with a malloc double-free error when destructing the second instance during shutdown.

The fix is to provide a custom = operator like this:

RingBuffer& operator= (const RingBuffer& other) {

        memcpy(elements, other.elements, capacity);
        r = other.r;
        w = other.w;

        return *this;
    }
mithrendal commented 3 months ago

very beautiful !

dirkwhoffmann commented 3 months ago

Fixed in v5.0b7