Lmms: JackAudio + JackMidi cause invalid and uninitialized reads

[JohannesLorenz]

Bug Summary

JackMidi causes invalid and uninitialized reads

Steps to reproduce

Enable JackMidi, restart LMMS.

When enabled: Open LMMS. Close LMMS.

Expected behavior

No memory errors.

Actual behavior

Invalid and uninitialized reads. Sometimes, this leads to crashes.

LMMS version used

master: eebdc0f

Logs

Click to expand

Startup

==21784== Thread 11:
==21784== Conditional jump or move depends on uninitialised value(s)
==21784==    at 0x3AAD4D: MidiJack::JackMidiRead(unsigned int) (MidiJack.cpp:180)
==21784==    by 0x3884C3: AudioJack::processCallback(unsigned int, void*) (AudioJack.cpp:347)
==21784==    by 0x3887D3: AudioJack::staticProcessCallback(unsigned int, void*) (AudioJack.cpp:429)
==21784==    by 0x712C2A9: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x712BA07: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x7144B1C: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x489F4CE: start_thread (in /usr/lib/libpthread-2.30.so)
==21784==    by 0x70432D2: clone (in /usr/lib/libc-2.30.so)
==21784== 
==21784== Conditional jump or move depends on uninitialised value(s)
==21784==    at 0x38856E: AudioJack::processCallback(unsigned int, void*) (AudioJack.cpp:382)
==21784==    by 0x3887D3: AudioJack::staticProcessCallback(unsigned int, void*) (AudioJack.cpp:429)
==21784==    by 0x712C2A9: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x712BA07: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x7144B1C: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x489F4CE: start_thread (in /usr/lib/libpthread-2.30.so)
==21784==    by 0x70432D2: clone (in /usr/lib/libc-2.30.so)



When closing:

==21784== Thread 11:
==21784== Invalid read of size 8
==21784==    at 0x3AACF2: MidiJack::JackMidiRead(unsigned int) (MidiJack.cpp:172)
==21784==    by 0x3884C3: AudioJack::processCallback(unsigned int, void*) (AudioJack.cpp:347)
==21784==    by 0x3887D3: AudioJack::staticProcessCallback(unsigned int, void*) (AudioJack.cpp:429)
==21784==    by 0x712C2A9: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x712BA07: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x7144B1C: ??? (in /usr/lib/libjack.so.0.1.0)
==21784==    by 0x489F4CE: start_thread (in /usr/lib/libpthread-2.30.so)
==21784==    by 0x70432D2: clone (in /usr/lib/libc-2.30.so)
==21784==  Address 0xa277770 is 160 bytes inside a block of size 440 free'd
==21784==    at 0x4839EAB: operator delete(void*) (vg_replace_malloc.c:586)
==21784==    by 0x3AAB1F: MidiJack::~MidiJack() (MidiJack.cpp:145)
==21784==    by 0x32EB69: Mixer::~Mixer() (Mixer.cpp:189)
==21784==    by 0x32ED23: Mixer::~Mixer() (Mixer.cpp:201)
==21784==    by 0x3057C3: void LmmsCore::deleteHelper<Mixer>(Mixer**) (Engine.h:141)
==21784==    by 0x3050D4: LmmsCore::destroy() (Engine.cpp:104)
==21784==    by 0x3DDF7C: MainWindow::~MainWindow() (MainWindow.cpp:260)
==21784==    by 0x3DDFF1: MainWindow::~MainWindow() (MainWindow.cpp:261)
==21784==    by 0x6AD9FFF: QObject::event(QEvent*) (in /usr/lib/libQt5Core.so.5.14.1)
==21784==    by 0x4A57488: QWidget::event(QEvent*) (in /usr/lib/libQt5Widgets.so.5.14.1)
==21784==    by 0x4B71574: QMainWindow::event(QEvent*) (in /usr/lib/libQt5Widgets.so.5.14.1)
==21784==    by 0x4A13361: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.14.1)
==21784==  Block was alloc'd at
==21784==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:344)
==21784==    by 0x331C8B: Mixer::tryMidiClients() (Mixer.cpp:1149)
==21784==    by 0x32EE47: Mixer::initDevices() (Mixer.cpp:216)
==21784==    by 0x304EA2: LmmsCore::init(bool) (Engine.cpp:79)
==21784==    by 0x3D2B3B: GuiApplication::GuiApplication() (GuiApplication.cpp:115)
==21784==    by 0x2A400A: main (main.cpp:815)

[JohannesLorenz]

• The first one was easy to fix, temporary fixed on lv2-review (0953718). I'll cherry-pick it later.
• The second one is a hard nut.
• The 3rd one (on closing) requires me to understand the code: MidiJack::MidiJack claims to not set the callback if the jack server is open? Aside from that the status parameter is not really checked, why should we not want a callback in this case?

[PhysSong]

• The second one is a hard nut.

We're not initializing AudioJack::m_stopped to true at construction. In other words, the variable remains uninitialized before the audio interface starts to run. It also applies to PortAudio and SDL.

The 3rd one is partially related to 17e87c1d68. MidiJack should set AudioJack::m_midiClient to null on destruction, but it doesn't.

[JohannesLorenz]

We're not initializing AudioJack::m_stopped to true at construction.

It's set to false though in startProcessing(), so how can it be uninitialized in processCallback()?

[PhysSong]

We're not initializing AudioJack::m_stopped to true at construction.

It's set to false though in startProcessing(), so how can it be uninitialized in processCallback()?

I don't know why, but AudioJack::processCallback() seems to be called before AudioJack::startProcessing() for some reason. Initializing AudioJack::m_stopped seems to fix the issue.
FYI, I got some messages about Mixer::m_fifoWriter being uninitialized when I was doing some tests for this issue.

[JohannesLorenz]

I don't know why, but AudioJack::processCallback() seems to be called before AudioJack::startProcessing() for some reason

Strange, I could confirm this in gdb. How is this possible? :confused:

[JohannesLorenz]

The bugs here are so harmful that I can't even start stable-1.2-LMMS with JACK anymore. Unfortunatelly, I'll have to add this to the stable-1.2 list.

I'll develop a PR the next days, but I still haven't fixed all the invalid reads.

[JohannesLorenz]

Analysis for (2): I set a breakpoint on jack_activate. It gets called by MidiJack::MidiJack() before ever being called by AudioJack:

#0  AudioJack::processCallback(unsigned int, void*) (this=0x555555fd6f10, _nframes=64, _udata=0x555555fd6f10) at /home/johannes/cprogs/lmms/stable-1.2/src/core/audio/AudioJack.cpp:344
#1  0x000055555575b50c in AudioJack::staticProcessCallback(unsigned int, void*) (_nframes=64, _udata=0x555555fd6f10) at /home/johannes/cprogs/lmms/stable-1.2/src/core/audio/AudioJack.cpp:433
#2  0x00007ffff57d92aa in  () at /usr/lib/libjack.so.0
#3  0x00007ffff57d8a08 in  () at /usr/lib/libjack.so.0
#4  0x00007ffff57f1b1d in  () at /usr/lib/libjack.so.0
#5  0x00007ffff7f6346f in start_thread () at /usr/lib/libpthread.so.0
#6  0x00007ffff591f3d3 in clone () at /usr/lib/libc.so.6

That explains why it's already activated before AudioJack activates it.

[JohannesLorenz]

I opened draft PR #5452 .

About criticality for 1.2:

• (1) has a fix in the PR
• (2) is critical (workaround is to not use JackMidi at all)
• (3) is not critical

[JohannesLorenz]

Analysis, point 2.

Mixer.cpp does this:

// called by LmmsCore::init:
m_audioDev = tryAudioDevices(); // 1
m_midiClient = tryMidiClients(); // 2
...
// called later by LmmsCore::init:
m_audioDev->startProcessing() // 3

(1) starts the AudioJack CTOR, which does not yet activate jack ((3) does that). (2) starts the MidiJack CTOR, which does activate jack already. This is why before (3), jack is already activated.

IMO the cleanest would be to split the processing into another virtual MidiJack::startProcessing, so audio and midi are started symmetrically. Dirty solution: I pushed it to #5452 (2nd commit).

[JohannesLorenz]

So now, 2 questions remain:

• Is the dirty fix for (2) sufficient for stable-1.2, or do we need a larger clean fix?
• Should (3) be fixed for stable-1.2? It crashes LMMS, but not before you close LMMS.

[PhysSong]

If you initialize AudioJack::m_stopped with true on construction, LMMS will stop crashing. If you can confirm it works, you can use it for stable-1.2.

• It crashes LMMS, but not before you close LMMS.

A crash is a crash. #2633 is an example.

[JohannesLorenz]

If you initialize AudioJack::m_stopped with true on construction, LMMS will stop crashing. If you can confirm it works, you can use it for stable-1.2.

This will silence the uninitialized read, but it will not fix the error that a callback is running that should not yet be run. I think that makes the code even worse, because it hides the underlying error.

For fixing (3), I'll have to look closer.

[PhysSong]

but it will not fix the error that a callback is running that should not yet be run.

Neither does your 'dirty' fix. Both mine and yours make the callback clear the buffer if the audio interface is not started yet.

[JohannesLorenz]

In a practical test, valgrind was quiet with your approach.

However, if we allow JackMidi to run the callback right in the moment where m_stopped gets false, the ports are unconnected yet (as that happens after m_stopped is set to false). Have you considered this issue?

Despite from how we solve that, m_stopped should be atomic. I'll fix that, too.

[PhysSong]

the ports are unconnected yet

I think that's a minor issue, and we may ignore that at least for stable-1.2. I agree that we should add more changes to master to fix the underlying issue.

[JohannesLorenz]

I now added a commit which uses m_stopped, but it doesn't set it to false before the ports are connected. The fix also sets m_stopped to true in the CTOR and makes it atomic.

Are you OK with this solution?

[PhysSong]

Go ahead. :rocket:
You may add a comment to indicate that we need some fundamental changes later.

[JohannesLorenz]

OK. I'd even write an issue report against master.

What fudamental changes do you have in mind, now that it's working with m_stopped?

[PhysSong]

What fudamental changes do you have in mind

I think we can make a wrapper class to manage the shared JACK client.

[JohannesLorenz]

I don't know how to describe that :)

I now fixed all 3 issues in #5452 . Aside from that comment, the PR is complete.

[Spekular]

Shouldn't this be closed now that the fix is merged to stable? Then a new issue can be created for the fundamental issues, that isn't milestoned for 1.2.2.

[zonkmachine]

Closed in #5452