Sviatoslav Razmyslov

Nov 27 2017

Tags:

#Cpp

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

• Introduction
• The Comma Operator (,)
• Miscellaneous Errors
• Few Examples from the Tests
• Comments on the code
• Conclusion

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.

Introduction

Steinberg GmbH (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.

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

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 Undefined behavior. The 'p' variable is modified while being used twice between sequence points. mdaAmbienceProcessor.cpp 151

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 The 'inputBitmap' pointer was utilized 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 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 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 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 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 an unobvious bug, which the analyzer has found. You can make yourself sure in it 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 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 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 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:

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 was exited without releasing 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 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 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 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 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 an uninitialized variable oldScrollSize. As it turned out, there is no error, but the implementation of the function getScrollSize() is horrible:

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

Certainly, such code would look better as follows:

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 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 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 The same value is assigned twice 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 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
• Part 2. Audacity
• Part 3. Rosegarden
• Part 4. Ardour

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.

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

#Cpp

0

0

0

0

SHARE

Tags:

#Cpp

We can email you a selection of our best articles once a month