Overview of defects in music software code. Part 2. Audacity

The series of articles on the review of defects in the code of musically software continues. The second challenger for analysis is the Audacity audio editor. This program is very popular and widely used by both amateurs and professionals in the music industry. In this article, a description of the code fragments will be additionally accompanied by popular memes. It will not be boring!

Introduction

Audacity is a free multi-platform audio file audio editor focused on working with multiple tracks. The program is open source and runs on operating systems such as Microsoft Windows, Linux, Mac OS X, FreeBSD and others.

Audacity actively uses third-party libraries, so the lib-src directory was excluded from the analysis , which contained about a thousand more files with the source code of different libraries. The article includes only the most interesting errors. To view the full report, authors can independently verify the project by requesting a temporary key in support.

PVS-StudioIs a tool for detecting errors in the source code of programs written in C, C ++ and C #. It works under Windows and Linux.

Copy-Paste - it is everywhere!

V523 The 'then' statement is equivalent to the 'else' statement. AButton.cpp 297

AButton::AButtonState AButton::GetState()
{
  ....
      if (mIsClicking) {
        state = mButtonIsDown ? AButtonOver : AButtonDown; //ok
      }
      else {
        state = mButtonIsDown ? AButtonDown : AButtonOver; //ok
      }
    }
  }
  else {
    if (mToggle) {
      state = mButtonIsDown ? AButtonDown : AButtonUp; // <= fail
    }
    else {
      state = mButtonIsDown ? AButtonDown : AButtonUp; // <= fail
    }
  }
  return state;
}

The above code snippet is an ideal contender for copying: just change the conditional expression and a couple of constants. Unfortunately, the author did not finish the job and left two identical code branches in the code.

Some more strange places:

• V523 The 'then' statement is equivalent to the 'else' statement. ASlider.cpp 394
• V523 The 'then' statement is equivalent to the 'else' statement. ExpandingToolBar.cpp 297
• V523 The 'then' statement is equivalent to the 'else' statement. Ruler.cpp 2422

Another example:

V501 There are identical sub-expressions 'buffer [remaining - WindowSizeInt - 2]' to the left and to the right of the '-' operator. VoiceKey.cpp 309

sampleCount VoiceKey::OnBackward (
   const WaveTrack & t, sampleCount end, sampleCount len)
{
  ....
  int atrend = sgn(buffer[remaining - 2]-buffer[remaining - 1]);
  int ztrend = sgn(buffer[remaining - WindowSizeInt - 2] -
                   buffer[remaining - WindowSizeInt - 2]);
  ....
}

The second time the sgn () function is called, the difference of the same values ​​is passed to it. Most likely, they wanted to get the difference between neighboring elements of the buffer, but forgot to change the deuce to one after copying a fragment of the line.

Misuse of functions

V530 The return value of function 'remove' is required to be utilized. OverlayPanel.cpp 31

bool OverlayPanel::RemoveOverlay(Overlay *pOverlay)
{
  const size_t oldSize = mOverlays.size();
  std::remove(mOverlays.begin(), mOverlays.end(), pOverlay);
  return oldSize != mOverlays.size();
}

Misuse of the std :: remove () function is so common that such an example is provided in the documentation for this diagnostic. Therefore, in order not to copy the description from the documentation again, I will simply give the corrected version:

bool OverlayPanel::RemoveOverlay(Overlay *pOverlay)
{
  const size_t oldSize = mOverlays.size();
  mOverlays.erase(std::remove(mOverlays.begin(), mOverlays.end(),
    pOverlay), mOverlays.end());
  return oldSize != mOverlays.size();
}

V530 The return value of function 'Left' is required to be utilized. ASlider.cpp 973

wxString LWSlider::GetTip(float value) const
{
  wxString label;
  if (mTipTemplate.IsEmpty())
  {
    wxString val;
    switch(mStyle)
    {
    case FRAC_SLIDER:
      val.Printf(wxT("%.2f"), value);
      break;
    case DB_SLIDER:
      val.Printf(wxT("%+.1f dB"), value);
      if (val.Right(1) == wxT("0"))
      {
        val.Left(val.Length() - 2);        // <=
      }
      break;
  ....
}

Here's what the prototype of the Left () function looks like :

wxString Left (size_t count) const

Obviously, the string val will not change. Most likely, they wanted to save the changed string back to val , but they did not read the documentation for the function.

PC users' nightmare

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. ExtImportPrefs.cpp 600

void ExtImportPrefs::OnDelRule(wxCommandEvent& WXUNUSED(event))
{
  ....
  int msgres = wxMessageBox (_("...."), wxYES_NO, RuleTable);
  if (msgres == wxNO || msgres != wxYES)
    return;
  ....
}

Many computer program users have ever clicked “wrong” and tried to undo the action ... So the error found in Audacity is that the condition that checks the pressed button in the dialog box does not depend on whether “No” was clicked or not: D

Here is what the truth table looks like for the given code fragment:

All such errors in the conditions are collected in the article " Logical expressions in C / C ++. How professionals are mistaken ."

“While” or “if”?

V612 An unconditional 'return' within a loop. Equalization.cpp 379

bool EffectEqualization::ValidateUI()
{
  while (mDisallowCustom && mCurveName.IsSameAs(wxT("unnamed")))
  {
    wxMessageBox(_("...."),
       _("EQ Curve needs a different name"),
       wxOK | wxCENTRE,
       mUIParent);
    return false;
  }
  ....
}

Here is a cycle written by the author, which runs 1 or 0 iterations. If there is no error, then it will be more obvious to rewrite the use of the if statement .

Using std :: unique_ptr

V522 Dereferencing of the null pointer 'mInputStream' might take place. FileIO.cpp 65

std::unique_ptr mInputStream;
std::unique_ptr mOutputStream;
wxInputStream & FileIO::Read(void *buf, size_t size)
{
   if (mInputStream == NULL) {
      return *mInputStream;
   }
   return mInputStream->Read(buf, size);
}
wxOutputStream & FileIO::Write(const void *buf, size_t size)
{
   if (mOutputStream == NULL) {
      return *mOutputStream;
   }
   return mOutputStream->Write(buf, size);
}

The analyzer detected very strange code. Pointer dereferencing occurs in any case, regardless of whether it is zero or not.

V607 Ownerless expression. LoadEffects.cpp 340

void BuiltinEffectsModule::DeleteInstance(IdentInterface *instance)
{
   // Releases the resource.
   std::unique_ptr < Effect > {
      dynamic_cast(instance)
   };
}

An example of a very interesting application of unique_ptr . This "one-liner" (excluding formatting) serves to create unique_ptr and destroy it right there, freeing the instance pointer .

Miscellaneous

V779 Unreachable code detected. It is possible that an error is present. ToolBar.cpp 706

void ToolBar::MakeRecoloredImage( teBmps eBmpOut, teBmps eBmpIn )
{
  // Don't recolour the buttons...
  MakeMacRecoloredImage( eBmpOut, eBmpIn );
  return;
  wxImage * pSrc = &theTheme.Image( eBmpIn );
  ....
}

The analyzer detected unreachable code due to the unconditional return statement in the code.

V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. ExportFFmpeg.cpp 229

#define AV_VERSION_INT(a, b, c) (a<<16 | b<<8 | c)
ExportFFmpeg::ExportFFmpeg() : ExportPlugin()
{
  ....
  int canmeta = ExportFFmpegOptions::fmts[newfmt].canmetadata;
  if (canmeta && (canmeta == AV_VERSION_INT(-1,-1,-1)  // <=
               || canmeta <= avfver))
  {
    SetCanMetaData(true,fmtindex);
  }
  ....
}

Intentionally make a shift in the negative number, which can lead to unobvious problems.

V595 The 'clip' pointer was utilized before it was verified against nullptr. Check lines: 4094, 4095. Project.cpp 4094

void AudacityProject::AddImportedTracks(....)
{
  ....
  WaveClip* clip = ((WaveTrack*)newTrack)->GetClipByIndex(0);
  BlockArray &blocks = clip->GetSequence()->GetBlockArray();
  if (clip && blocks.size())
  {
    ....
  }
  ....
}

In this condition , it is too late to check the clip pointer ; it was dereferenced with the line above.

A few more dangerous places:

• V595 The 'outputMeterFloats' pointer was utilized before it was verified against nullptr. Check lines: 5246, 5255. AudioIO.cpp 5246
• V595 The 'buffer2' pointer was utilized before it was verified against nullptr. Check lines: 404, 409. Compressor.cpp 404
• V595 The 'p' pointer was utilized before it was verified against nullptr. Check lines: 946, 974. ControlToolBar.cpp 946
• V595 The 'mParent' pointer was utilized before it was verified against nullptr. Check lines: 1890, 1903. LV2Effect.cpp 1890

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: true. TimeTrack.cpp 296

void TimeTrack::WriteXML(XMLWriter &xmlFile) const
{
  ....
  // MB: so why don't we just call Invalidate()? :)
  mRuler->SetFlip(GetHeight() > 75 ? true : true);
  ....
}

It seems that one of the developers guessed that such a code does not make sense, but instead of fixing the code, I decided to comment on it.

V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '! j-> hasFixedBinCount' and 'j-> hasFixedBinCount'. LoadVamp.cpp 169

wxArrayString VampEffectsModule::FindPlugins(....)
{
  ....
  if (.... ||
      !j->hasFixedBinCount ||
      (j->hasFixedBinCount && j->binCount > 1))
 {
   ++output;
   continue;
 }
 ....
}

The condition is redundant. It can be simplified to this option:

!j->hasFixedBinCount || j->binCount > 1

And another example of this code:

• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '! j-> hasFixedBinCount' and 'j-> hasFixedBinCount'. LoadVamp.cpp 297

Conclusion

It is unlikely that such errors will be noticeable to the end listener, but for Audacity users this can cause great inconvenience.

Other reviews:

• Overview of defects in music software code. Part 1. MuseScore
• Overview of defects in music software code. Part 3. Rosegarden

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 .

Trying the PVS-Studio analyzer on your project is very easy, 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. Code Defects in Music Software. Part 2. Audacity