Qtractor LV2 worker implementation bug

[swesterfeld]

I can reproducibly crash Qtractor from git by loading my SpectMorph plugin and generating lots of UI changes with the mouse (using the 2x2 grid template). I debugged the problem, and the crash is
caused by the function qtractorLv2Worker::schedule, which in the original implementation uses:

::jack_ringbuffer_write(m_pRequests, (const char *) &size, sizeof(size));
::jack_ringbuffer_write(m_pRequests, (const char *) data, size);

to write the request. Now the case that triggers the crash is that the first ringbuffer_write is executed, and then the request is read by the worker thread. Since data isn't yet written, the worker thread executes an incomplete request, which in turn causes a crash in the SpectMorph plugin. I know it sounds like an unlikely thing to happen, but I can trigger it reproducibly in a short amount of time.

My fix is to copy the data into one consecutive chunk of data, and writing that with one single ringbuffer_write. Patch attached.

Cu... Stefan
lv2workerfix.diff.txt

[rncbc]

On 09/22/2016 08:19 PM, swesterfeld wrote:

My fix is to copy the data into one consecutive chunk of data, and
writing that with one single ringbuffer_write. Patch attached.

Cu... Stefan
lv2workerfix.diff.txt
https://github.com/rncbc/qtractor/files/488177/lv2workerfix.diff.txt

made its way to git head master (aka. qtractor v0.7.9.2)
0bb4d55
https://gitlab.com/rncbc/qtractor/commit/0bb4d55
https://bitbucket.org/rncbc/qtractor/commits/0bb4d55
http://sourceforge.net/p/qtractor/code/ci/0bb4d5

please test && tell (and possibly close)

thanks

rncbc aka. Rui Nuno Capela

[swesterfeld]

Looks reasonable, except for one detail (which breaks all requests as you committed it):

if (::jack_ringbuffer_write_space(m_pRequests) < request_size) {

should be ... >= request_size

As to whether to inform the user somehow if the out-of-space-condition occurred (which assert() would have done), it's probably a matter of taste whether to fail silently - as you implemented it - or produce some sort of warning.

[rncbc]

On 09/22/2016 10:49 PM, swesterfeld wrote:

Looks reasonable, except for one detail (which breaks all requests as
you committed it):

if (::jack_ringbuffer_write_space(m_pRequests) < request_size) {

should be ... >= request_size

of course. 7d31291
https://gitlab.com/rncbc/qtractor/commit/7d31291
https://bitbucket.org/rncbc/qtractor/commits/7d31291
http://sourceforge.net/p/qtractor/code/ci/7d3129
thanks.

As to whether to inform the user somehow if the out-of-space-condition
occurred (which assert() would have done), it's probably a matter of
taste whether to fail silently - as you implemented it - or produce some
sort of warning.

when the later sort of warning comes like a dang crash/abort, then i
prefer the former ;)

cheers

rncbc aka. Rui Nuno Capela

[swesterfeld]

Right, now everything works for me, it no longer crashes with my testcase, so I'm closing the issue.

As for producing a warning without crash/abort, you could add something like

fprintf (stderr, "qtractorLv2Worker::schedule failed, ring buffer full\n");

This would at least give users/developers a clue about what happened if a plugin is misbehaving. Anyway that's up to you.