synth: Prevent data from being freed twice in AsyncPlugInCmd

[weefuzzy]

Purpose and Motivation

Without this, World_Free is called on the same memory twice, as SequencedCmd also frees this pointer. This then causes the FIFO queue on the thread to get into trouble and render the server unresponsive.

Fixes #4338

Types of changes

• Bug fix

To-do list

• Code is tested
• All tests are passing
• [n/a] Updated documentation
• This PR is ready for review

[mossheim]

Thanks! Can you please reformat this so it passes linting in our CI, @weefuzzy?

Full instructions are here: https://github.com/supercollider/supercollider/wiki/Cpp-formatting-instructions#linting-and-formatting

But if you just want to get this PR ready for merge, all you need to do is change the destructor to this:

AsyncPlugInCmd::~AsyncPlugInCmd() { (mCleanup)(mWorld, mCmdData); }

[weefuzzy]

@brianlheim apologies, I'd totally missed the formatting requirements. Thanks for the quick fix, I'm on the move and upgrading clang seems ambitious for public wifi, so I've gone for the manual change.

[sonoro1234]

Caveat: There are other derived classes of SequencedCmd that call World_Free on destructor.
Perhaps would be better to rework World_Free (void AllocPool::Free(void* inPtr)) https://github.com/supercollider/supercollider/blob/develop/common/SC_AllocPool.cpp#L147

so that:

• sets inPtr=NULL after freeing
• returns without doing anything if inPtr==NULL (which is already done except for DISABLE_MEMORY_POOLS)

[weefuzzy]

Thanks @sonoro1234 . The issue isn't calling World_Free in and of itself, so much as calling it on the same pointer, mMsgDataa twice: the other derived classes of SequencedCmd don't try and free the completion message string and instead leave it to the parent destructor. This PR brings ASyncPluginCmd into harmony with its siblings in that respect.

I can't comment much on the suggested changes to World_Free. They sound perfectly reasonable, but presumably affect a great deal of other code. Even if it did behave this way, it would still seem stylistically redundant to have a descendent and a parent free the same pointer in their destructors.

[sonoro1234]

the other derived classes of SequencedCmd don't try and free the completion

https://github.com/supercollider/supercollider/blob/develop/server/scsynth/SC_SequencedCommand.cpp#L305
https://github.com/supercollider/supercollider/blob/develop/server/scsynth/SC_SequencedCommand.cpp#L458

and others

[weefuzzy]

BufGenCmd is freeing a pointer mData, not mMsgData
BufAllocReadCmd is freeing a pointer mFilename, again no mMsgData
Both these classes seem to own these pointers that they are freeing, and meanwhile do not attempt to to tidy the resources associated with the completion message.

[sonoro1234]

You are right!!
Also: Just tested and check that works as intended.

[sonoro1234]

Could test that it works as stated.

@snappizz should I merge this? squash merge?

[mossheim]

@sonoro1234 Yes, this should be squash merged. Thanks for testing & reviewing!