File load causes SIGSEGV in ControllerView

[groboclown]

When loading some of my older save files, they cause a crash as described in the following backtrace.

I uploaded an example of one of the files that crash at http://groboclown.net/music/lmms/controllerview-crash.mmpz

#0  0x0000000000000000 in ?? ()
#1  0x00000000005d37df in ControllerView::ControllerView (this=0x19bbe60, 
    _model=0x125e410, _parent=0xf0fc80)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/gui/widgets/ControllerView.cpp:64
#2  0x00000000005d2b1d in ControllerRackView::onControllerAdded (
    this=0xf0cde0, controller=0x125e410)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/gui/widgets/ControllerRackView.cpp:149
#3  0x0000000000614157 in ControllerRackView::qt_static_metacall (_o=0xf0cde0, 
    _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7fffffffc9d0)
    at /home/me/software/lmms-git/groboclown-lmms.github/build/src/moc_ControllerRackView.cpp:56
#4  0x00007ffff68a9c28 in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib64/qt4/libQtCore.so.4
#5  0x000000000061ecc5 in Song::controllerAdded (this=0xa47240, _t1=0x125e410)
    at /home/me/software/lmms-git/groboclown-lmms.github/build/src/moc_Song.cpp:226
#6  0x0000000000545cd3 in Song::addController (this=0xa47240, 
    controller=0x125e410)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/core/Song.cpp:1400
#7  0x0000000000544a24 in Song::restoreControllerStates (this=0xa47240, 
    element=...)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/core/Song.cpp:1207
#8  0x00000000005436e0 in Song::loadProject (this=0xa47240, fileName=...)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/core/Song.cpp:1002
#9  0x0000000000592364 in MainWindow::openRecentlyOpenedProject (
    this=0xaac1b0, _action=0x14b3a10)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/gui/MainWindow.cpp:869
#10 0x0000000000619ffd in MainWindow::qt_static_metacall (_o=0xaac1b0, 
    _c=QMetaObject::InvokeMetaMethod, _id=29, _a=0x7fffffffcc90)
    at /home/me/software/lmms-git/groboclown-lmms.github/build/src/moc_MainWindow.cpp:138
#11 0x00007ffff68a9c28 in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib64/qt4/libQtCore.so.4
#12 0x00007ffff7479522 in QMenu::triggered(QAction*) ()
   from /usr/lib64/qt4/libQtGui.so.4
#13 0x00007ffff747a950 in QMenuPrivate::_q_actionTriggered() ()
   from /usr/lib64/qt4/libQtGui.so.4
#14 0x00007ffff68a9c28 in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib64/qt4/libQtCore.so.4
#15 0x00007ffff7014fe2 in QAction::triggered(bool) ()
   from /usr/lib64/qt4/libQtGui.so.4
#16 0x00007ffff7016af3 in QAction::activate(QAction::ActionEvent) ()
   from /usr/lib64/qt4/libQtGui.so.4
#17 0x00007ffff74797d9 in QMenuPrivate::activateCausedStack(QList<QPointer<QWidget> > const&, QAction*, QAction::ActionEvent, bool) ()
   from /usr/lib64/qt4/libQtGui.so.4
#18 0x00007ffff747e279 in QMenuPrivate::activateAction(QAction*, QAction::ActionEvent, bool) () from /usr/lib64/qt4/libQtGui.so.4
#19 0x00007ffff7073cb8 in QWidget::event(QEvent*) ()
   from /usr/lib64/qt4/libQtGui.so.4
#20 0x00007ffff748291b in QMenu::event(QEvent*) ()
   from /usr/lib64/qt4/libQtGui.so.4
#21 0x00007ffff701b791 in QApplicationPrivate::notify_helper(QObject*, QEvent*)
    () from /usr/lib64/qt4/libQtGui.so.4
#22 0x00007ffff7023ff0 in QApplication::notify(QObject*, QEvent*) ()
   from /usr/lib64/qt4/libQtGui.so.4
#23 0x00007ffff689440d in QCoreApplication::notifyInternal(QObject*, QEvent*)
    () from /usr/lib64/qt4/libQtCore.so.4
#24 0x00007ffff7021e42 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) ()
   from /usr/lib64/qt4/libQtGui.so.4
#25 0x00007ffff709e32c in QETWidget::translateMouseEvent(_XEvent const*) ()
   from /usr/lib64/qt4/libQtGui.so.4
#26 0x00007ffff709c9d1 in QApplication::x11ProcessEvent(_XEvent*) ()
   from /usr/lib64/qt4/libQtGui.so.4
#27 0x00007ffff70c4872 in x11EventSourceDispatch(_GSource*, int (*)(void*), void*) () from /usr/lib64/qt4/libQtGui.so.4
#28 0x00007ffff3dee704 in g_main_context_dispatch ()
   from /usr/lib64/libglib-2.0.so.0
#29 0x00007ffff3dee948 in g_main_context_iterate.isra ()
   from /usr/lib64/libglib-2.0.so.0
#30 0x00007ffff3dee9ec in g_main_context_iteration ()
   from /usr/lib64/libglib-2.0.so.0
#31 0x00007ffff68c3f9e in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4
#32 0x00007ffff70c4976 in QGuiEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtGui.so.4
#33 0x00007ffff68927cf in QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4
#34 0x00007ffff6892a75 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/qt4/libQtCore.so.4
#35 0x00007ffff6898a59 in QCoreApplication::exec() ()
   from /usr/lib64/qt4/libQtCore.so.4
#36 0x00000000004d759d in main (argc=1, argv=0x7fffffffdde8)
    at /home/me/software/lmms-git/groboclown-lmms.github/src/core/main.cpp:677

[Wallacoloo]

I have encountered this one as well. The offending line is (ControllerView.cpp:64):

QLabel *label = new QLabel( "<b>" + _model->displayName() + "</b>", this);

And it can occur when _model != NULL and this != NULL. _model->displayName() is a simple getter function that shouldn't error under normal operation, IIRC. I know I found a use-after-free error around the same area, so it may be related. All of the LMMS memory management stuff is done manually though, so it gets really difficult to tell who's supposed to be the owner of an object at any given time. It's kind of a mess.

Anyway, if anyone can reproduce the error under Valgrind, that ought to reveal a bit more about what's happening.

[michaelgregorius]

@Wallacoloo I think the crash is caused in Song::restoreControllerStates which looks as follows in master:

void Song::restoreControllerStates( const QDomElement & element )
{
    QDomNode node = element.firstChild();
    while( !node.isNull() )
    {
        Controller * c = Controller::create( node.toElement(), this );
        Q_ASSERT( c != NULL );

        /* For PeakController, addController() was called in
         * PeakControllerEffect::PeakControllerEffect().
         * This line removes the previously added controller for PeakController
         * without affecting the order of controllers in Controller Rack
         */
        Engine::getSong()->removeController( c );
        addController( c );

        node = node.nextSibling();
    }
}

So let's see what happens here:

1. A controller is created from the XML.
2. The controller is removed immediately from the Song using Song::removeController. Here we should already be scratching our heads. This is a fresh controller. Why should it be in the Song already? Normally, it should be created and then added to the song. Fortunately, the comment above this line helps us with this question. But let's continue with the flow of execution.
3. Song::removeController checks whether the controller is in the list of managed controllers. If it is in the list it is removed from the list and delete is called on the controller. This will happen for Peak Controllers because PeakControllerEffect in fact calls Song::addController in it's constructor as alluded to by the comment. All other controllers do not add themselves so nothing is removed or deleted in these cases.
4. Song::addController is called with the new controller. For all other controllers this will be ok but in the case of a Peak Controller it will add an instance on which delete has already been called.

The main problem here is that some code has been added to the core class Song to make sure that a plugin that does not seem to adhere to the specification / protocol works. In my opinion this needs to be fixed in PeakControllerEffect.

[Wallacoloo]

@michaelgregorius I definitely agree with your last sentiment. And coincidentally, the bit of code you pasted above was the same use-after-free I was thinking of.

[softrabbit]

The controller is removed immediately from the Song using Song::removeController. Here we should already be scratching our heads.

Frankly, I'm scratching at the Engine::getSong()-> part already. Why ask the engine for a Song from a method in Song? And why not do the same in the next line as well? You could end up operating on different Songs, right?

I'd avoid re-adding the controller by doing something like this for now and shooting for purity in PeakControllerEffect later:

if( m_controllers.indexOf( c ) == -1 )
{
    addController( c );
}

[michaelgregorius]

@softrabbit Yes, that Engine::getSong() call is strange indeed. Especially since LMMS is not able to handle more than one song anyway.

Where would you add the code snippet you have provided? In Song::restoreControllerStates? Only asking because that would mean that the Peak Controller from the file would not be added, wouldn't it?

[softrabbit]

Where would you add the code snippet you have provided? In Song::restoreControllerStates? Only asking because that would mean that the Peak Controller from the file would not be added, wouldn't it?

That code was totally wrong after my half-assed copy & paste-work, so I updated the earlier comment to something closer to what I intended. I was thinking those lines would replace the 2 odd lines in Song::restoreControllerStates that seem to be to main point of interest here. IOW, instead of "remove, then add", it'd be "if the Song doesn't have that Controller already, add it".

But now that I look again at what's going on, I realize Song::addController already checks and doesn't add a controller that's already added, so just removing the engine::getSong()->removeController( c ); line should do what I was aiming for.

Looking at the methods involved (https://github.com/LMMS/lmms/blob/master/src/core/Song.cpp#L1395) I don't see why the Peak Controller wouldn't be added. And after commenting out that aforementioned line I can save a project using the Peak Controller and load it just fine (i.e. it controls the knob it's supposed to).

Funny, the source of that odd logic is pull #225 that was meant to fix a bug with a pretty similar description...

[tresf]

removing the engine::getSong()->removeController( c ); line should do what I was aiming for.

@softrabbit Thanks. Are you comfortable enough with that statement to submit a patch?