MemoryManager: Null pointer deallocation attempted #2023

Wallacoloo · 2015-04-28T04:37:22Z

Steps to reproduce:

Open LMMS & be presented with the default template: 1 3xOSC track, 1 Sample Track, 1 Beat/Bassline track and 1 Automation track.
Click on any bar in the row of the song editor corresponding to the sample track (this should create a 1 bar long waveform that is just a flat line).
Undo the action with ctrl+z or by using the edit menu.
Redo the action with ctrl+y or by using the edit menu.

After step 4, the text "MemoryManager: Null pointer deallocation attempted" should be printed to the console.

This affects current master (so e.g. it's definitely not due to the unmerged #2022 PR that modifies undo/redo shortcuts)

The text was updated successfully, but these errors were encountered:

Wallacoloo · 2015-04-28T04:47:40Z

And here's a backtrace for all you lovely folks:

#0  MemoryManager::free (ptr=0x0) at /home/colin/proj/lmms/src/core/MemoryManager.cpp:110
#1  0x000000000053c408 in SampleBuffer::loadFromBase64 (this=0x7fffeb093160, _data=...) at /home/colin/proj/lmms/src/core/SampleBuffer.cpp:1362
#2  0x0000000000623d14 in SampleTCO::loadSettings (this=0x7fffeb092ea0, _this=...) at /home/colin/proj/lmms/src/tracks/SampleTrack.cpp:186
#3  0x0000000000623eef in non-virtual thunk to SampleTCO::loadSettings(QDomElement const&) () at /home/colin/proj/lmms/src/tracks/SampleTrack.cpp:190
#4  0x0000000000541e23 in SerializingObject::restoreState (this=0x7fffeb092ec0, element=...) at /home/colin/proj/lmms/src/core/SerializingObject.cpp:70
#5  0x00000000005139a7 in JournallingObject::restoreState (this=0x7fffeb092ec0, _this=...) at /home/colin/proj/lmms/src/core/JournallingObject.cpp:92
#6  0x0000000000552171 in Track::loadSettings (this=0x7fffeb08c0a0, element=...) at /home/colin/proj/lmms/src/core/Track.cpp:2031
#7  0x000000000055248f in non-virtual thunk to Track::loadSettings(QDomElement const&) () at /home/colin/proj/lmms/src/core/Track.cpp:2044
#8  0x0000000000541e23 in SerializingObject::restoreState (this=0x7fffeb08c0c0, element=...) at /home/colin/proj/lmms/src/core/SerializingObject.cpp:70
#9  0x00000000005139a7 in JournallingObject::restoreState (this=0x7fffeb08c0c0, _this=...) at /home/colin/proj/lmms/src/core/JournallingObject.cpp:92
#10 0x0000000000535538 in ProjectJournal::redo (this=0xa6c310) at /home/colin/proj/lmms/src/core/ProjectJournal.cpp:92
#11 0x000000000059ba49 in MainWindow::redo (this=0xb113c0) at /home/colin/proj/lmms/src/gui/MainWindow.cpp:1234
#12 0x0000000000634eb5 in MainWindow::qt_static_metacall (_o=0xb113c0, _c=QMetaObject::InvokeMetaMethod, _id=25, _a=0x7fffffffcec0)
    at /home/colin/proj/lmms/build-debug/src/moc_MainWindow.cpp:131
#13 0x00007ffff6963a7a in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#14 0x00007ffff70f3a54 in QShortcut::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#15 0x00007ffff70c411c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#16 0x00007ffff70ca870 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#17 0x00007ffff694f86d in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#18 0x00007ffff70f5cb3 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#19 0x00007ffff70f5e24 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#20 0x00007ffff70cc09a in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#21 0x00007ffff694f86d in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#22 0x00007ffff71649e0 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#23 0x00007ffff7164e62 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#24 0x00007ffff713e48c in QApplication::x11ProcessEvent(_XEvent*) () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#25 0x00007ffff7167432 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#26 0x00007ffff39acc5d in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#27 0x00007ffff39acf48 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#28 0x00007ffff39acffc in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#29 0x00007ffff697d04e in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#30 0x00007ffff71674e6 in ?? () from /usr/lib/x86_64-linux-gnu/libQtGui.so.4
#31 0x00007ffff694e4f1 in QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#32 0x00007ffff694e805 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#33 0x00007ffff6953f67 in QCoreApplication::exec() () from /usr/lib/x86_64-linux-gnu/libQtCore.so.4
#34 0x00000000004d13ec in main (argc=1, argv=0x7fffffffe4a8) at /home/colin/proj/lmms/src/core/main.cpp:542

Wallacoloo · 2015-04-28T04:56:49Z

Also, this error is triggered by a part of the SampleBuffer.cpp code that's conditionally compiled. It can only be reached when building without FLAC (i.e. LMMS_HAVE_FLAC_STREAM_DECODER_H is undefined).

(edit: I incorrectly linked to line 1355 instead of 1362 of SampleBuffer.cpp)

tresf · 2015-04-28T05:10:14Z

FYI - This was mostly committed here 9c25be1#diff-cc740586f26342e6e68642925c6fc52bR61 as part of the memmgr improvements. @diizy wrote this almost entirely and I believe it was first introduced in stable-1.1

Don't know if that's much help...

Off-topic, but I found these lines interesting SampleBuffer.cpp#L1068-1069

Wallacoloo · 2015-04-28T07:19:26Z

@tresf well those lines are interesting! So then I guess this error ought to be present on builds regardless of FLAC support.

Umcaruje · 2015-07-05T21:38:56Z

Confirmed on the current master.

tresf · 2015-09-28T14:49:04Z

@Wallacoloo this console error also displays when trying to reproduce #2378.

My console displays this:

MemoryManager: Null pointer deallocation attempted
called Track::getTCO( 6 ), but TCO 6 doesn't exist
QObject: Cannot create children for a parent that is in a different thread.
(Parent is InstrumentTrack(0x7f4624132220), parent's thread is QThread(0xef49d0), current thread is QThread(0x12e6bc0)

This bug was opened previously against the wrong branch here #1352 and probably prematurely closed, since it was valid for master.

michaelgregorius · 2015-09-30T18:58:13Z

I think the null pointer deallocation should be safe to fix by changing from MM_FREE( m_origData ); to

if ( m_origData )
{
    MM_FREE( m_origData );
}

in SampleBuffer::loadFromBase64.

Here's what happens:

When the sample is created the constructor of SampleBuffer is called. It's called in the version that only sets m_origData to 0. There follows no other code that initializes m_origData to any other value.
The undo is executed. To store the state SampleBuffer::toBase64 is called from within SampleTCO::saveSettings. This method in turn only delegates to base64::encode which does not use or alter m_origData in any way (so it is still 0).
During the redo another SampleBuffer is created again using the version that initializes m_origData to 0. Later SampleTCO::loadSettings is called which in turn calls SampleBuffer::loadFromBase64 which performs the deallocation in question.

So there seem to be valid path through SampleBuffer which do not allocate something for m_origData. Therefore it should also be safe to add the additional check.

Another question is whether the check if( ptr == NULL ) in MemoryManager::free is "legit" in the first place. After all writing this code is perfectly legal in C++:

Something * s = new Something();
delete s;
s = 0;
delete s;

It is not nice but legal. So it might be that SampleBuffer was written with that design in mind. Later on the memory management code was introduced which checks that no null pointer deletion is executed and suddenly SampleBuffer looks like a bad guy. 😉

tresf · 2015-09-30T19:48:54Z

I think the null pointer deallocation should be safe to fix by changing from MM_FREE( m_origData ); to
if ( m_origData )
{
    MM_FREE( m_origData );
}

This same null check technique is used all over the place in SampleBuffer.cpp, so I would assume it is a reasonable expectation. I think we'll have to do it twice though due to the #else a bit below.

So we can assume this bug is perfectly benign in this context, right?

Wallacoloo · 2015-09-30T20:15:16Z

Given that MM_FREE and MM_ALLOC are supposed to behave like free and alloc, respectively, it seems the fix is to just not warn when one attempts to delete a null pointer (as free(NULL) is a valid operation), and keep everything else the same. i.e. remove MemoryManager.cpp:110

But yes, my understanding is that this bug is benign.

Also, I'm curious about the necessity of a "memory manager", which appears to just provide an alternative implementation of malloc/free. IMO, if glibc's malloc/free are too slow for lmms, it seems like it would be more productive to use a drop-in replacement like TCmalloc (as an example), rather than rolling our own.

tresf · 2015-09-30T20:23:26Z

Also, I'm curious about the necessity of a "memory manager", which appears to just provide an alternative implementation of malloc/free. IMO, if glibc's malloc/free are too slow for lmms, it seems like it would be more productive to use a drop-in replacement like TCmalloc (as an example), rather than rolling our own.

Yeah, as far as I remember, benchmarks weren't conclusive whether or not rolling our own MemoryManager were warranted, here's the conversation thread... #1088

and also more conversation here (perhaps what inspired it)... #1053 (comment)

free(NULL) is a valid operation [...] But yes, my understanding is that this bug is benign.

Hiding the warning is probably a good idea then..

grejppi · 2015-09-30T22:05:25Z

@Wallacoloo IIRC the point of MemoryManager is to provide a realtime safe way to allocate memory in a DSP thread for example, something new and malloc() and friends aren't suitable for

michaelgregorius · 2015-10-01T20:59:08Z

IIRC the point of MemoryManager is to provide a realtime safe way to allocate memory in a DSP thread for example, something new and malloc() and friends aren't suitable for

The problem is that MemoryManager implicitly calls malloc in MemoryManager::extend (which is potentially called from MemoryManager::alloc) which kind of defeats its whole purpose with regards to realtime safety.

The cleanest (but obviously also hardest) solution would be to get rid of dynamic allocations wherever possible and to make sure that the remaining dynamic allocations are not performed in the audio processing thread.

Here is some interesting read on the things that should not be done in the audio thread: http://www.rossbencina.com/code/real-time-audio-programming-101-time-waits-for-nothing

softrabbit · 2015-10-09T07:02:18Z

What's critical about this issue, really? It's not like LMMS doesn't spew out a few other warnings as well. Disabling it for release builds should be enough IMO. (Thought qDebug() was supposed to work that way, but looks like it doesn't for some reason)

tresf · 2015-10-09T14:07:19Z

What's critical about this issue, really?

It's probably not after further investigation. Since it often happens before a crash, it was bumped to critical, but further investigation seems to suggest it is purely cosmetic... or to quote @Wallacoloo...

my understanding is that this bug is benign.

Critical priority removed. 👍

Spekular · 2015-10-09T14:37:18Z

So should it be bumped to 1.3?

tresf · 2015-10-09T14:43:08Z

So should it be bumped to 1.3?

Not necessarily since the fix is probably going to be removing a debug statement. 👍

Removes the warning "MemoryManager: Null pointer deallocation attempted" from MemoryManager. The warning does not make sense because deallocations of null pointers are ok in C++. Fixes LMMS#2023.

michaelgregorius · 2015-10-10T20:11:55Z

Fixed with pull request #2406.

Umcaruje added bug core labels Jul 5, 2015

tresf mentioned this issue Sep 29, 2015

Crash while playing and undo-ing #2389

Closed

tresf added the critical label Sep 29, 2015

tresf added this to the 1.2.0 milestone Sep 29, 2015

tresf removed the critical label Oct 9, 2015

michaelgregorius mentioned this issue Oct 10, 2015

Removes an unnecessary warning in MemoryManager::free #2406

Merged

Wallacoloo closed this as completed in #2406 Oct 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MemoryManager: Null pointer deallocation attempted #2023

MemoryManager: Null pointer deallocation attempted #2023

Wallacoloo commented Apr 28, 2015

Wallacoloo commented Apr 28, 2015

Wallacoloo commented Apr 28, 2015

tresf commented Apr 28, 2015

Wallacoloo commented Apr 28, 2015

Umcaruje commented Jul 5, 2015

tresf commented Sep 28, 2015

michaelgregorius commented Sep 30, 2015

tresf commented Sep 30, 2015

Wallacoloo commented Sep 30, 2015

tresf commented Sep 30, 2015

grejppi commented Sep 30, 2015

michaelgregorius commented Oct 1, 2015

softrabbit commented Oct 9, 2015

tresf commented Oct 9, 2015

Spekular commented Oct 9, 2015

tresf commented Oct 9, 2015

michaelgregorius commented Oct 10, 2015

MemoryManager: Null pointer deallocation attempted #2023

MemoryManager: Null pointer deallocation attempted #2023

Comments

Wallacoloo commented Apr 28, 2015

Wallacoloo commented Apr 28, 2015

Wallacoloo commented Apr 28, 2015

tresf commented Apr 28, 2015

Wallacoloo commented Apr 28, 2015

Umcaruje commented Jul 5, 2015

tresf commented Sep 28, 2015

michaelgregorius commented Sep 30, 2015

tresf commented Sep 30, 2015

Wallacoloo commented Sep 30, 2015

tresf commented Sep 30, 2015

grejppi commented Sep 30, 2015

michaelgregorius commented Oct 1, 2015

softrabbit commented Oct 9, 2015

tresf commented Oct 9, 2015

Spekular commented Oct 9, 2015

tresf commented Oct 9, 2015

michaelgregorius commented Oct 10, 2015