# Review of Music Software Code Defects\. Part 5\. Steinberg SDKs

I continue to review the code of musical applications, and here is the first representative of commercial software\. Reading the comments to the previous articles, I noticed the popularity of Cubase and decided to read about it\. This is the product of a Steinberg company, which has several programs with closed source code\. I accidentally found the SDK for third\-party developers on their website, and, after studying it, found a lot of interesting bugs\.

![0541_MusicSoftwareDefects_05_SteinbergSDKs/image1.png](https://import.viva64.com/docx/blog/0541_MusicSoftwareDefects_05_SteinbergSDKs/image1.png)

## Introduction

[Steinberg GmbH](https://www.steinberg.net/en/home.html) \(Steinberg Media Technologies GmbH\) is a German musical software and equipment company based in Hamburg\. It mainly produces music recording, arranging and editing software as used in digital audio workstations and VSTi software synthesizers\. Steinberg is a wholly owned subsidiary of [Yamaha Corporation](https://www.yamaha.com/)\.

![0541_MusicSoftwareDefects_05_SteinbergSDKs/image2.png](https://import.viva64.com/docx/blog/0541_MusicSoftwareDefects_05_SteinbergSDKs/image2.png)

One review article is actually not enough even for a small number of source code from the SDK, so to view the full report, code authors may independently check the project having sent a request for a temporary key to our support to evaluate the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) analyzer abilities\. It is a tool for bug detection in the source code of programs, written in C, C\+\+, and C\#\. It works in Windows and Linux environment\.

## The Comma Operator \(,\)

The comma operator \(,\) is used to execute expressions standing on the two sides from it from left to right and to get the value of the **right **expression\. Most often, the operator is applied to an expression for the changes counter of the_ for_ loop\. Sometimes it is convenient to use it in the debugging and testing macros\. Nevertheless, most frequently, developers excessively and incorrectly use it\.

[V521](https://pvs-studio.com/en/docs/warnings/v521/) Such expressions using the ',' operator are dangerous\. Make sure the expression 'i < temp, i < numParams' is correct\. mdaBaseProcessor\.cpp 309

```cpp
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 misuse of the comma operator\. It is not clear, what the author of the code wanted to say by using it\. The code looks like innocuous, so let's move on to the next example\.

[V521](https://pvs-studio.com/en/docs/warnings/v521/) Such expressions using the ',' operator are dangerous\. Make sure the expression is correct\. mdaBaseProcessor\.cpp 142

```cpp
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
    {
      ....
    }
    ....
  }
  ....
}
```

A serious mistake has been made here\. In the loop a developer accesses the arrays _data\.inputs _and _data\.outputs_, but the conditional expression is written with an error\. Though the expression _bus < data\.numInputs _is calculated, it doesn't affect the result\. Therefore, accessing memory out of bounds of an array _data\.inputs_ is possible\.

I specifically gave two examples to show that one of the developers is abusing the use of this operator and makes mistakes\.

## Miscellaneous Errors

[V567](https://pvs-studio.com/en/docs/warnings/v567/) Undefined behavior\. The 'p' variable is modified while being used twice between sequence points\. mdaAmbienceProcessor\.cpp 151

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

The analyzer has detected expressions that result in undefined behavior of a program\. The variables are repeatedly used between two sequence points, while their values are changing\. As a result, it is impossible to predict the result of the work of such an expression\. In general, 11 similar fragments were found\.

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'inputBitmap' pointer was utilized before it was verified against nullptr\. Check lines: 409, 410\. cbitmapfilter\.cpp 409

```cpp
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 pointer _inputBitmap_ is compared with _nullptr_ immediately after use\. A developer wanted to check pointer _inputBitmap_ and variable _radius_ in one condition, but it is impossible to do so, as one value is calculated using another one\. You must check each variable separately\.

[V1004](https://pvs-studio.com/en/docs/warnings/v1004/) The 'module' pointer was used unsafely after it was verified against nullptr\. Check lines: 76, 84\. audiohost\.cpp 84

```cpp
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 a _module _was equal to _NULL_, the function would be interrupted by calling _kill\(\)_\. Now a call of this function is commented out, so there is now a risk of a null pointer dereference\.

[V766](https://pvs-studio.com/en/docs/warnings/v766/) An item with the same key '0xff9b' has already been added\. x11frame\.cpp 51

```cpp
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 an unobvious bug, which the analyzer has found\. You can make yourself sure in it only when viewing the preprocessor output:

```cpp
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 of _0xff9b_\. Unfortunately, it is not clear what to do with it, because the constants are taken from the GDK3 library\.

## Few Examples from the Tests

[V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(vstPlug\)' condition was already verified in line 170\. vsttestsuite\.cpp 172

```cpp
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](https://pvs-studio.com/en/docs/warnings/v571/) diagnostic simply finds the excess checks, but, apparently, here is a real bug\. I looked through the similar fragments in the file and, most likely, the code should be fixed as follows:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'paramIds' pointer\. A memory leak is possible\. vsttestsuite\.cpp 436

```cpp
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 function _run\(\)_ has more than a dozen of exit points where a memory leak occurs\. Memory freeing for this array by the pointer _paramIds_ will be performed only when the function is executed until the end\.

## Comments on the code

[V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. mdaJX10Processor\.cpp 522

```cpp
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 a conditional operator began to carry out similar actions\. It is difficult to say whether this leads to an error or maybe you can now just get rid of the check\. So, this fragment is worth checking out and rewriting more clearly\.

[V573](https://pvs-studio.com/en/docs/warnings/v573/) Uninitialized variable 'oldScrollSize' was used\. The variable was used to initialize itself\. cscrollview\.cpp 482

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

The analyzer detected a potential use of an uninitialized variable _oldScrollSize_\. As it turned out, there is no error, but the implementation of the function _getScrollSize\(\)_ is horrible:

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

Certainly, such code would look better as follows:

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

Couple more similar 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](https://pvs-studio.com/en/docs/warnings/v751/) Parameter 'column' is not used inside function body\. pitchnamesdatabrowsersource\.cpp 227

```cpp
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 column number that was passed to the function is not used in the function _dbCellTextChanged\(\)_\. It is difficult for me to say, whether there is a bug or not, so the authors of the project should recheck the code\.

[V570](https://pvs-studio.com/en/docs/warnings/v570/) The same value is assigned twice to the 'lpf' variable\. mdaComboProcessor\.cpp 274

```cpp
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 comment on code: there is an unnecessary assignment of variable _lpf_\. Most likely, this is a typo, randomly not causing an error\.

## Conclusion

Steinberg SDKs contains different sources, including examples of plugins\. Detected errors might reflect the condition of code of other company's products with the closed source code\.

Here is my opinion on the issue, which code is better \- close or open\. It is very simple\. Code quality depends more on a project manager rather than on its privacy\. It is much easier to deal with open source code: it is easier to report an error, users may add features or fix a bug\.\.\. Nevertheless, if a project manager doesn't make it available to use quality control methods, code will not get better\.  You should definitely use both all available free solutions and, if possible, add checks by paid tools\.

Other music software reviews:

* [Part 1\. MuseScore](https://pvs-studio.com/en/blog/posts/cpp/0530/)
* [Part 2\. Audacity](https://pvs-studio.com/en/blog/posts/cpp/0532/)
* [Part 3\. Rosegarden](https://pvs-studio.com/en/blog/posts/cpp/0536/)
* [Part 4\. Ardour](https://pvs-studio.com/en/blog/posts/cpp/0540/)

If you know an interesting software to work with music and want to see it in review, then send me the names of the programs by [mail](mailto:razmyslov@viva64.com)\.

It is very simple to try PVS\-Studio analyzer on your project, just go to the [download](https://pvs-studio.com/en/pvs-studio/download/) page\.