rncbc / qjackctl

QjackCtl - JACK Audio Connection Kit Qt GUI Interface
https://qjackctl.sourceforge.io
GNU General Public License v2.0
183 stars 41 forks source link

Memory corruption in graph when disconnecting #59

Closed jpcima closed 5 years ago

jpcima commented 5 years ago

Hi. There is a crash in the graph editor in a particular situation.

It seems in relation with client name. In this repository is a flanger plugin realized with faust2lv2: https://github.com/linuxmao-org/greffons-faust It's an educational-purpose source with non-english text, containing accented characters in the name. When it's instantiated, it seems to give qjackctl graph editor some trouble. (rev 17ef7ce)

These are my steps to produce a crash:

  1. open in Jalv the plugin by URI https://github.com/linuxmao-org/greffons-faust/flanger
  2. connect something on the instance's first input by qjackctl graph editor
  3. disconnect the connection just created
  4. it's probably crashing here, otherwise repeat 2. until it does

In valgrind, a memory error will be reported; see vg-log.txt.gz

rncbc commented 5 years ago

does it only happen with the ports of that plugin; maybe because of UTF-8 client/port names (accentuated non-ascii chars)?

what happens if you get rid of those (problematic) characters from the faust flanger.dsp source? like trading "Stéréo" for "Stereo" (no accents)?

jpcima commented 5 years ago

The first time I have ever encountered such crashes has been with this client, so that's at least where my initial suspicion is. I can reproduce it easily in current situation, let me try to extract quickly some more elaborate information off my debug build.

jpcima commented 5 years ago

Look, it has a programming problem here.

https://github.com/rncbc/qjackctl/blob/17ef7ce5ea3eb830fde4a0237deef542f377ce0d/src/qjackctlJackConnect.cpp#L243-L244

As you can see, it takes constData from a temporary QByteArray instance which is destroyed right after.

rncbc commented 5 years ago

destroyed when? i believe it is only destroyed when going out of scope, which should happen a lot later than its immediate and legit use right after, on the very next line, as in https://github.com/rncbc/qjackctl/blob/17ef7ce5ea3eb830fde4a0237deef542f377ce0d/src/qjackctlJackConnect.cpp#L243-L246 ...

otoh. this code does not concern to the Graph but rather to the (more than decade old) Connections widget.

jpcima commented 5 years ago

sClientName.toUtf8() has return type QByteArray, which is a value return, not reference. The instance will be dead after leaving the statement. It's verifiable in valgrind, which raises at this point. But, it's indeed unrelated and did not resolve the issue in question.


About the actual one: it was not relevant to characters, as accent removal did not produce any effect.

Inside qjackctlGraphForm::refresh, there will be deletion of a graph item at updateItems, and the same item which was just deleted will be accessed deeper into the call stabilize following right after (as QGraphicsView's currentItem).

if (m_jack)
    m_jack->updateItems();
stabilize();
rncbc commented 5 years ago

sClientName.toUtf8() has return type QByteArray, which is a value return, not reference. The instance will be dead after leaving the statement. It's verifiable in valgrind, which raises at this point.

i see your point and you're of course right. maybe fixed now in https://github.com/rncbc/qjackctl/commit/89a6b4

thanks.

jpcima commented 5 years ago

I just check the latest, it works perfectly now. Thanks very much