Crash/segfault when enabling/disabling chatty MIDI controllers

[Sean M. Pappalardo]

When disabling a very chatty controller (moving platter) and enabling another (non-chatty, doesn't matter which) I get a segfault. This happens with no tracks loaded. (The SCS.1d is the controller in question in this case and it constantly sends timestamp messages even when it's stopped. FWIW, these are Sysex messages that are 18 bytes long.)

Steps to reproduce:
1) Connect and turn on a very chatty controller
2) Start Mixxx.
3) Go to prefs and enable said controller, disabling all others. Click OK.
4) Go back to prefs, un-check Enable on the chatty controller, check Enable on another (like Midi-Through) and click OK.
5) Observe the segfault in Pm_Poll().

Happens in 1.11 r3447 & 3450.

[Sean M. Pappalardo (pegasus-renegadetech)]

Attached another back trace where the crash happened right after disabling one controller while enabling another.

[RJ Skerry-Ryan (rryan)]

Thanks for the backtraces. It looks like PM is segfaulting with no other Mixxx thread doing anything suspicious.

Do you compile PortMIDI by hand? There are no debug symbols for PortMIDI in your backtrace. Having those would be useful to track it down.

[RJ Skerry-Ryan (rryan)]

I assume this doesn't happen in 1.10.x?

[RJ Skerry-Ryan (rryan)]

Ah, I think I see the issue. When you close a device the m_pInputStream and m_pOutputStreams are not NULL'd so we are pointing to free'd memory. If it goes like:

uncheck activate -> PortMidiController::close() -> m_pInputStream is now pointing to invalid memory
poll timer activates -> ControllerManager -> PortMidiController::poll() -> Pm_Poll with a freed input stream -> segfault

Added a potential fix in lp:mixxx/1.11 r3450. Sean -- can you try it and confirm?

[Sean M. Pappalardo (pegasus-renegadetech)]

No, I use the version of PortMIDI packaged by Debian Squeeze, 184. I don't see a package that would include debug symbols.
Your change didn't seem to help the situation much. The attached backtrace looks much the same.

I can't reproduce in 1.10, but it has numerous other problems with buffering MIDI messages. (It will segfault when PortMidi overflows.)

[RJ Skerry-Ryan (rryan)]

Did you test my fix?

[RJ Skerry-Ryan (rryan)]

Sorry, didn't read the title of your latest attachment.

[RJ Skerry-Ryan (rryan)]

When does the segfault happen? Right after clicking 'ok' on the preferences? Can you isolate it to just using a single midi controller or does it require multiple controllers?

I can't reproduce on a mac (to simulate the motorized controller I just moved a jog constantly).

[RJ Skerry-Ryan (rryan)]

Could you remover the debian portmidi and build by hand with debug symbols? Also, it doesn't look like a corruption backtrace so when you get a crash in gdb w/ PM debug symbols, poke around in the various stack frames to see what various variables are and if everything looks right. You may need to compile with no optimizations otherwise a lot of temporaries will be optimized out.

Also, it would help if you could provide some more of the log before the backtrace.

[Sean M. Pappalardo (pegasus-renegadetech)]

Not sure what to look for here. It seems like maybe we're still polling when we ought not to be. Race condition? The attached crash happened when I disabled the SCS.1d (which is very chatty) and enabled the .1m at the same time.

[Sean M. Pappalardo (pegasus-renegadetech)]

Here's one more for you. I rebuilt PortMIDI with its Verbose flag on. The log is from the point that I un-checked Enabled on the SCS.1d and checked it on the MIDI-Through device. PM sends some messages to the device on shutdown which is expected, and it appears to close fine. The segfault happens when the newly-opened device is polled, so maybe this is a bug in PortMIDI?

[Sean M. Pappalardo (pegasus-renegadetech)]

Also interesting is that the crash still happens even if you reverse the order of opening and closing devices: I tried enabling a device beneath the SCS.1d (the .1m in this case) and even though the controller polling is stopped then restarted, the segfault still occurs. It also happens if I disable two devices and enable one, as long as the .1d is one of the disabled devices.

More interestingly, it also happens every time if I have two devices enabled (SCS.1d and another) and disable _only the SCS.1d_. (Back trace looks the same though.)

[Sean M. Pappalardo (pegasus-renegadetech)]

And this seems to fix it!

[Sean M. Pappalardo (pegasus-renegadetech)]

Cross-reference: https://sourceforge.net/apps/trac/portmedia/ticket/3

[RJ Skerry-Ryan (rryan)]

Nice job!

Since our ControllerManager thread inherently prevents any parallelism in how we talk to PortMIDI it isn't possible that we are calling Pm_Poll() before we have finished a Pm_Close on the controller in question.

After reading PortMIDI for a little bit I know why it requires 2 devices to trigger now:

After closing one device and opening another:
1) Chatty MIDI device sends new messages on the wire, ALSA queues them up for that MIDI port.
2) Pm_Close() on the chatty device marks the descriptor closed, NULLs internalDescriptor (the pointer you check for NULL in your patch). Messages for chatty-device's port are still queued in ALSA since we haven't received them yet.
3) Mixxx starts polling for the newly opened device,
4) Pm_Poll() called on new device. Pm_Poll would exit early if we called it on a closed device so Mixxx is definitely not calling it on the recently closed device. This is why 2 devices are required.
5) Even though you call Pm_Poll on a specific device, Pm_Poll gets messages for all MIDI ports with pending messages.

As you pointed out in your patch, handle_event assumes we will only receive messages for devices that are open so it doesn't check internalDescriptor for NULL.

I think PortMIDI's ALSA module has a variety of NULL-pointer issues but they are all guarded against by the common API since the common API will generally bail on any operation that you try to do when the device is not open.

I noticed that in alsa_in_close, after pm_free'ing midi->descriptor, it does not clear midi->descriptor. That means that if we were to somehow be able to call any method in PortMIDI that uses midi->descriptor on the input port, it would cause a segfault. The corresponding alsa_out_close method does clear midi->descriptor. The fix for this is simple:

--- pmlinuxalsa.c 2012-11-02 10:46:48.481183991 -0400
+++ pmlinuxalsa.c 2012-11-02 10:42:45.341228735 -0400
@@ -339,6 +339,7 @@
         pm_hosterror = snd_seq_delete_port(seq, desc->this_port);
     }
     alsa_unuse_queue();
+ midi->descriptor = NULL;
     pm_free(desc);
     if (pm_hosterror) {
         get_alsa_error_text(pm_hosterror_text, PM_HOST_ERROR_MSG_LEN,

[RJ Skerry-Ryan (rryan)]

The state of affairs here is that we are waiting for PortMIDI to commit the fix and then we can try to get the Debian maintainer to repackage it.

[RJ Skerry-Ryan (rryan)]

The fix is now in PortMIDI r226 so I've gotten in touch with piem (the portmidi package maintainer) to update it.

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "pmlinuxalsa.c patch to prevent using null pointer in handle_event()" of this bug report has been identified as being a patch. The ubuntu-reviewers team has been subscribed to the bug report so that they can review the patch. In the event that this is in fact not a patch you can resolve this situation by removing the tag 'patch' from the bug report and editing the attachment so that it is not flagged as a patch. Additionally, if you are member of the ubuntu-reviewers team please also unsubscribe the team from this bug report.

[This is an automated message performed by a Launchpad user owned by Brian Murray. Please contact him regarding any issues with the action taken in this bug report.]

[Alessio Treglia (quadrispro)]

[Impact]

 * It affects users who rely on Portmidi.

 * When disabling a very chatty controller (moving platter) and enabling another (non-chatty, doesn't matter which) you get a segfault. This happens with no tracks loaded. (The SCS.1d is the controller in question in this case and it constantly sends timestamp messages even when it's stopped. FWIW, these are Sysex messages that are 18 bytes long.)

[Test Case]

1) Connect and turn on a very chatty controller
2) Start Mixxx.
3) Go to prefs and enable said controller, disabling all others. Click OK.
4) Go back to prefs, un-check Enable on the chatty controller, check Enable on another (like Midi-Through) and click OK.
5) Observe the segfault in Pm_Poll().

[Regression Potential]

The patch is minimalistic, there could be no regression at all.

[Other Info]

The patch was already uploaded to Debian unstable to fix an RC bug.

[RJ Skerry-Ryan (rryan)]

Thank you Alessio! I'm marking Fix Released in Mixxx since this is fixed in Debian and Ubuntu Raring w/ backports on the way for Quantal and below.

[Alessio Treglia (quadrispro)]

BTW, the patch has been accepted upstream too.

[Brian Murray (brian-murray)]

Hello Sean, or anyone else affected,

Accepted portmidi into quantal-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/portmidi/1:200-0ubuntu1.12.10.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Brian Murray (brian-murray)]

Hello Sean, or anyone else affected,

Accepted portmidi into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/portmidi/1:200-0ubuntu1.12.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Brian Murray (brian-murray)]

Hello Sean, or anyone else affected,

Accepted portmidi into oneiric-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/portmidi/1:200-0ubuntu1.11.10.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Brian Murray (brian-murray)]

Hello Sean, or anyone else affected,

Accepted portmidi into lucid-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/portmidi/1:200-0ubuntu1.10.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Alessio Treglia (quadrispro)]

Confirmed working on Precise and Quantal.

Thanks.

[Colin Watson (cjwatson)]

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package portmidi - 1:200-0ubuntu1.12.04.1

---------------
portmidi (1:200-0ubuntu1.12.04.1) precise-proposed; urgency=low

  * debian/patches/11-pmlinuxalsa.patch:
    - Avoid SIGSEGV when it receives data for devices which
      might have already been closed. (LP: #1073484)
    - Fix some other pointer issues:
      + alsa_in_close() didn't clear midi-descriptor.
      + Some other uses of midi->descriptor didn't do NULL-check of
        the pointer.
 -- Alessio Treglia <email address hidden> Sun, 23 Dec 2012 22:14:44 +0000

[Colin Watson (cjwatson)]

Any chance of verification on lucid and oneiric too?

[Launchpad Janitor (janitor)]

This bug was fixed in the package portmidi - 1:200-0ubuntu1.12.10.1

---------------
portmidi (1:200-0ubuntu1.12.10.1) quantal-proposed; urgency=low

  * debian/patches/11-pmlinuxalsa.patch:
    - Avoid SIGSEGV when it receives data for devices which
      might have already been closed. (LP: #1073484)
    - Fix some other pointer issues:
      + alsa_in_close() didn't clear midi-descriptor.
      + Some other uses of midi->descriptor didn't do NULL-check of
        the pointer.
 -- Alessio Treglia <email address hidden> Sun, 23 Dec 2012 22:14:44 +0000

[Alessio Treglia (quadrispro)]

Just to confirm the patch works also on Oneiric and Lucid.

[Colin Watson (cjwatson)]

Thanks!

[Launchpad Janitor (janitor)]

This bug was fixed in the package portmidi - 1:200-0ubuntu1.10.04.1

---------------
portmidi (1:200-0ubuntu1.10.04.1) lucid-proposed; urgency=low

  * debian/patches/11-pmlinuxalsa.patch:
    - Avoid SIGSEGV when it receives data for devices which
      might have already been closed. (LP: #1073484)
    - Fix some other pointer issues:
      + alsa_in_close() didn't clear midi-descriptor.
      + Some other uses of midi->descriptor didn't do NULL-check of
        the pointer.
 -- Alessio Treglia <email address hidden> Sun, 23 Dec 2012 22:25:35 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package portmidi - 1:200-0ubuntu1.11.10.1

---------------
portmidi (1:200-0ubuntu1.11.10.1) oneiric-proposed; urgency=low

  * debian/patches/11-pmlinuxalsa.patch:
    - Avoid SIGSEGV when it receives data for devices which
      might have already been closed. (LP: #1073484)
    - Fix some other pointer issues:
      + alsa_in_close() didn't clear midi-descriptor.
      + Some other uses of midi->descriptor didn't do NULL-check of
        the pointer.
 -- Alessio Treglia <email address hidden> Sun, 23 Dec 2012 22:25:35 +0000

[Albert Graef (dr-graef)]

Alessio, could you please revisit the portmidi update that you released a few days ago? It's a minor glitch, but the libportmidi.so from the 12.04.1 update isn't properly linked and breaks Python+PortMidi applications such as Frescobaldi. Full bug report with suggested patch here: https://bugs.launchpad.net/ubuntu/+source/portmidi/+bug/1110326

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6680