Review of music software code defects. Part 5. Steinberg SDKs

I continue to review the code of music applications, and we have the first representative of commercial software. In the comments to previous articles, I noticed the popularity of the program Cubase and decided to read about it. This is a product of Steinberg, which has several closed source programs. Randomly, I found a third-party SDK on their website, and, having studied it, I discovered many interesting errors.

Introduction

Steinberg GmbH (Steinberg Media Technologies GmbH) is a German company based in Hamburg that develops music software and hardware. To a large extent, it produces software for recording, arranging and editing music for use on both digital audio workstations and synthesizers with VSTi format software. Steinberg is a subsidiary of Yamaha Corporation , Steinberg is fully owned by Yamaha Corporation.

Even for a small number of source codes from the SDK, there will be little one review article, so to view the full report, the authors can independently check the project by requesting a support key to evaluate the capabilities of the PVS-Studio static analyzer. It is a tool for detecting errors in the source code of programs written in C, C ++ and C # languages. Works in Windows and Linux environments.
')

Operator "comma" (,)

The comma operator (,) is used to execute expressions on both sides of it in the order from left to right and get the value of the right expression. Most often, an operator is used in a counter change expression for a for loop. Sometimes it is convenient to use it in debugging and testing macros. But most often this operator is abused and misused.

V521 Such expressions using the ',' operator are dangerous. Make sure the expression 'i <temp, i <numParams' is correct. mdaBaseProcessor.cpp 309

tresult PLUGIN_API BaseProcessor::setState (IBStream* state) { .... // read each parameter for (uint32 i = 0; i < temp, i < numParams; i++) { state->read (&params[i], sizeof (ParamValue)); SWAP64_BE(params[i]) } .... } 

A small example of improper use of the operator "comma". It is not clear what the author of the code wanted to say. The code seems to be harmless, so let's move on to the following example.

V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. mdaBaseProcessor.cpp 142

 bool BaseProcessor::bypassProcessing (ProcessData& data) { .... for (int32 bus = 0; bus < data.numInputs, // <= bus < data.numOutputs; bus++) { .... if (data.numInputs <= bus || data.inputs[bus].numChannels <= channel) { memset(data.outputs[bus].channelBuffers32[channel], ....); data.outputs[bus].silenceFlags |= (uint64)1 << channel; } else { .... } .... } .... } 

Here a serious mistake has already been made. The loop refers to the data.inputs and data.outputs arrays , but the conditional expression is written with an error. The expression bus <data.numInputs, although computed, does not affect the result. Therefore, it is possible to access the memory beyond the data.inputs array.

I specifically cited two examples to show that one of the programmers is abusing the use of this operator and makes mistakes.

Mistakes

V567 Undefined behavior. The sequence of the variable has been changed. mdaAmbienceProcessor.cpp 151

 void AmbienceProcessor::doProcessing (ProcessData& data) { .... ++p &= 1023; ++d1 &= 1023; ++d2 &= 1023; ++d3 &= 1023; ++d4 &= 1023; .... } 

The analyzer detected expressions that lead to undefined program behavior. Variables are repeatedly used between two sequence points, and their values ​​change. As a result, it is impossible to predict the result of the work of such an expression. Total found about 11 such places.

V595 The 'inputBitmap' pointer was used before it was verified against nullptr. Check lines: 409, 410. cbitmapfilter.cpp 409

 bool run (bool replace) override { CBitmap* inputBitmap = getInputBitmap (); uint32_t radius = static_cast<uint32_t>(static_cast<double>( .... * inputBitmap->getPlatformBitmap()->getScaleFactor()); if (inputBitmap == nullptr || radius == UINT_MAX) return false; .... } 

The inputBitmap pointer is compared to nullptr immediately after use. The programmer wanted to check the inputBitmap pointer and the radius variable in one condition, but this is impossible, because one value is calculated by the other. It is necessary to check each variable separately.

V1004 The 'module' pointer was used unsafely after it was verified against nullptr. Check lines: 76, 84. audiohost.cpp 84

 void App::startAudioClient (....) { std::string error; module = VST3::Hosting::Module::create (path, error); if (!module) { std::string reason = "Could not create Module for file:"; reason += path; reason += "\nError: "; reason += error; // EditorHost::IPlatform::instance ().kill (-1, reason); } auto factory = module->getFactory (); .... } 

Previously, if the module pointer was zero, the function was interrupted by calling kill () . Now the call to this function is commented out, so there is a risk of dereferencing the null pointer.

V766 An item with the same key '0xff9b' has already been added. x11frame.cpp 51

 using VirtMap = std::unordered_map<guint, uint16_t>; const VirtMap keyMap = { {GDK_KEY_BackSpace, VKEY_BACK}, {GDK_KEY_Tab, VKEY_TAB}, {GDK_KEY_ISO_Left_Tab, VKEY_TAB}, {GDK_KEY_Clear, VKEY_CLEAR}, {GDK_KEY_Return, VKEY_RETURN}, {GDK_KEY_Pause, VKEY_PAUSE}, {GDK_KEY_Escape, VKEY_ESCAPE}, {GDK_KEY_space, VKEY_SPACE}, {GDK_KEY_KP_Next, VKEY_NEXT}, // <= {GDK_KEY_End, VKEY_END}, {GDK_KEY_Home, VKEY_HOME}, {GDK_KEY_Left, VKEY_LEFT}, {GDK_KEY_Up, VKEY_UP}, {GDK_KEY_Right, VKEY_RIGHT}, {GDK_KEY_Down, VKEY_DOWN}, {GDK_KEY_Page_Up, VKEY_PAGEUP}, {GDK_KEY_Page_Down, VKEY_PAGEDOWN}, {GDK_KEY_KP_Page_Up, VKEY_PAGEUP}, {GDK_KEY_KP_Page_Down, VKEY_PAGEDOWN}, // <= .... }; 

Here is a non-obvious error found by the analyzer. This can be seen only when viewing the preprocessor output:

 using VirtMap = std::unordered_map<guint, uint16_t>; const VirtMap keyMap = { {0xff08, VKEY_BACK}, {0xff09, VKEY_TAB}, {0xfe20, VKEY_TAB}, {0xff0b, VKEY_CLEAR}, {0xff0d, VKEY_RETURN}, {0xff13, VKEY_PAUSE}, {0xff1b, VKEY_ESCAPE}, {0x020, VKEY_SPACE}, {0xff9b, VKEY_NEXT}, // <= {0xff57, VKEY_END}, {0xff50, VKEY_HOME}, {0xff51, VKEY_LEFT}, {0xff52, VKEY_UP}, {0xff53, VKEY_RIGHT}, {0xff54, VKEY_DOWN}, {0xff55, VKEY_PAGEUP}, {0xff56, VKEY_PAGEDOWN}, {0xff9a, VKEY_PAGEUP}, {0xff9b, VKEY_PAGEDOWN}, // <= .... }; 

Indeed, the constants GDK_KEY_KP_Next and GDK_KEY_KP_PageDown have the same value 0xff9b . That's just not clear what to do with it, because constants are taken from the GDK3 library.

Some examples from tests

V571 Recurring check. The 'if (vstPlug)' condition was already verified in line 170. vsttestsuite.cpp 172

 bool VstTestBase::teardown () { if (vstPlug) { if (vstPlug) { vstPlug->activateBus (kAudio, kInput, 0, false); vstPlug->activateBus (kAudio, kOutput, 0, false); } plugProvider->releasePlugIn (vstPlug, controller); } return true; } 

Quite often, the V571 diagnostics simply find the extra checks, but here, apparently, the real error. I looked at similar fragments in the file and, most likely, you should fix the code like this:

 bool VstTestBase::teardown () { if (plugProvider) // <= { if (vstPlug) { vstPlug->activateBus (kAudio, kInput, 0, false); vstPlug->activateBus (kAudio, kOutput, 0, false); } plugProvider->releasePlugIn (vstPlug, controller); } return true; } 

V773 The function of the paramIds pointer. A memory leak is possible. vsttestsuite.cpp 436

 bool PLUGIN_API VstScanParametersTest::run (....) { .... int32* paramIds = new int32[numParameters]; bool foundBypass = false; for (int32 i = 0; i < numParameters; ++i) { ParameterInfo paramInfo = {0}; tresult result = controller->getParameterInfo (i, paramInfo); if (result != kResultOk) { addErrorMessage (testResult, printf ("Param %03d: is missing!!!", i)); return false; // Memory Leak } int32 paramId = paramInfo.id; paramIds[i] = paramId; if (paramId < 0) { addErrorMessage (testResult, printf ("Param %03d: Invalid Id!!!", i)); return false; // Memory Leak } .... if (paramIds) delete[] paramIds; return true; } 

The run () function has more than ten exit points where a memory leak occurs. Only when executing the function until the end will the memory be cleared for this array according to the paramIds pointer.

Remarks on the code

V523 The 'then' statement is equivalent to the 'else' statement. mdaJX10Processor.cpp 522

 void JX10Processor::noteOn (....) { .... if (!polyMode) //monophonic retriggering { voice[v].env += SILENCE + SILENCE; } else { //if (params[15] < 0.28f) //{ // voice[v].f0 = voice[v].f1 = voice[v].f2 = 0.0f; // voice[v].env = SILENCE + SILENCE; // voice[v].fenv = 0.0f; //} //else voice[v].env += SILENCE + SILENCE; //anti-glitching trick } .... } 

After commenting on the part of the code, the branches of the conditional operator began to perform the same actions. It is difficult to say whether this leads to an error or now you can just get rid of the check. Therefore, this place is worth checking and rewriting more clearly.

V573 Uninitialized variable 'oldScrollSize' was used. The variable was used to initialize itself. cscrollview.cpp 482

 void CScrollView::setContainerSize (....) { CRect oldSize (containerSize); .... CRect oldScrollSize = vsb->getScrollSize (oldScrollSize); float oldValue = vsb->getValue (); .... } 

The analyzer detected a potential use of the uninitialized variable oldScrollSize . As it turned out, there will be no error, but the implementation of the getScrollSize () function is terrible:

 CRect& getScrollSize (CRect& rect) const { rect = scrollSize; return rect; } 

Surely, such a code would look better:

 CRect oldScrollSize = vsb->getScrollSize(); .... CRect& getScrollSize () const { return scrollSize; } 

A few more such initializations:

• V573 Uninitialized variable 'oldScrollSize' was used. The variable was used to initialize itself. cscrollview.cpp 503
• V573 Uninitialized variable 'oldClip' was used. The variable was used to initialize itself. ctabview.cpp 359

V751 Parameter 'column' is not used inside function body. pitchnamesdatabrowsersource.cpp 227

 void PitchNamesDataBrowserSource::dbCellTextChanged( int32_t row, int32_t column, ....) { if (pitchnames) { UString128 str (newText); if (str.getLength () == 0) pitchnames->removePitchName (0, (int16)row); else pitchnames->setPitchName (0, (int16)row, str); } } 

The dbCellTextChanged () function does not use the number of the column that was passed to the function. It's hard for me to say if there is a mistake or not, therefore the authors of the project should double-check the code.

V570 The same value is assigned to the 'lpf' variable. mdaComboProcessor.cpp 274

 void ComboProcessor::recalculate () { .... case 4: trim = 0.96f; lpf = filterFreq(1685.f); mix1 = -0.85f; mix2 = 0.41f; del1 = int (getSampleRate () / 6546.f); del2 = int (getSampleRate () / 3315.f); break; case 5: trim = 0.59f; lpf = lpf = filterFreq(2795.f); // <= mix1 = -0.29f; mix2 = 0.38f; del1 = int (getSampleRate () / 982.f); del2 = int (getSampleRate () / 2402.f); hpf = filterFreq(459.f); break; .... } 

A small note on the code: in the code there is an extra assignment to the variable lpf . Most likely, this is a typo, not randomly leading to an error.

Conclusion

Steinberg SDKs contains various sources, including examples of plugins. The errors found may reflect the status of the code of other closed source company products.

My position on which code is better, open or closed, is very simple. The quality of the code is more dependent on the project manager than on the closed / open code. With open source, of course, it’s good: it’s easy to report a bug, someone adds functionality, someone fixes a mistake ... but if the manager doesn’t ensure that quality control techniques are used in the project, it will not be better. You must use all available free solutions and add paid tools to them whenever possible.

Other music software reviews:

• Part 1. MuseScore
• Part 2. Audacity
• Part 3. Rosegarden
• Part 4. Ardour

If you know an interesting software for working with music and want to see it in the review, then send the names to me by mail .

It is very easy to try the PVS-Studio analyzer on your project, just go to the download page.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Review of Music Software Defects Part 5. Steinberg SDKs