To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Review of Music Software Code Defects. …

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

Nov. 27, 2017

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

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.

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 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:

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.

Popular related articles
The Evil within the Comparison Functions

Date: 05.19.2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.21.2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: 10.22.2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
PVS-Studio for Java

Date: 01.17.2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Last Line Effect

Date: 05.31.2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
The way static analyzers fight against false positives, and why they do it

Date: 03.20.2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.14.2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept