Sviatoslav Razmyslov

Oct 11 2017

Tags:

#Cpp

Review of Music Software Code Defects. Part 2. Audacity

• Introduction
• Copy-Paste - it's everywhere!
• Incorrect use of functions
• PC users' nightmare
• "while" or "if"?
• Using std::unique_ptr
• Miscellaneous
• Conclusion

We are going on with our series of articles about defects in audio software. The second project that was picked for analysis is Audacity audio editor. This program is highly popular and widely used by both amateurs and professionals. In this article, the comments on code fragments will be accompanied by some popular memes. It's going to be fun!

Introduction

Audacity is free, open-source, cross-platform audio software for multi-track recording and editing. It is available for Microsoft Windows, Linux, macOS X, FreeBSD, and other operating systems.

Audacity extensively uses third-party libraries, so we excluded the lib-src directory with about a thousand of source files of the various libraries from analysis. This article discusses only the most interesting defects. To see the complete log, the project authors are welcome to email us for a temporary registration key so they can check the project themselves.

PVS-Studio is a tool for detecting defects in the source code of software written in C, C++, and C#. The analyzer supports Windows and Linux operating systems.

Copy-Paste - it's 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;
}

This code fragment is a perfect candidate for copying: you just have to change the conditional expression and a couple of constants. Sadly, the author failed to see it through and created two identical code branches.

A few other suspicious fragments:

• 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]);
  ....
}

When calling the sgn() function for the second time, it receives as an argument the difference between identical values. The programmer must have meant it to be the difference between two adjacent elements of the buffer but forgot to change 2 to 1 after cloning the fragment of the string.

Incorrect use 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();
}

The std::remove() function is misused so often that we even included this example in the documentation to illustrate the diagnostic. Since you can find the description there, here's just the fixed version of the code:

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

This is what the prototype of the Left() function looks like:

wxString Left (size_t count) const

It's obvious that the val string won't be changed. The programmer must have meant to write the modified string back to val but hadn't read the documentation on 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 software users had at least once a situation when they misclicked and tried to cancel. Well, there's one nice error in Audacity: because of it, the condition checking which button was clicked in the dialog window doesn't actually depend on whether or not the button "No" was clicked :D

Here's the truth table for this code:

All errors of this type are discussed in the article "Logical Expressions in C/C++. Mistakes Made by Professionals".

"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;
  }
  ....
}

This cycle iterates either 1 or 0 times. If there's no error here, it's better to rewrite this code with an if statement.

Using std::unique_ptr

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

std::unique_ptr<wxInputStream> mInputStream;
std::unique_ptr<wxOutputStream> 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);
}

This one is a very strange code fragment. The pointer is dereferenced anyway, no matter if it's null or not.

V607 Ownerless expression. LoadEffects.cpp 340

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

This is an example of a very interesting way to use unique_ptr. This "one-liner" (we are not taking the formatting into account) is used to create unique_ptr only to destroy it right away, freeing the instance pointer along the way.

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 has detected a code fragment that can't be reached because of the unconditional return statement.

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);
  }
  ....
}

The programmer is deliberately shifting a negative number, which may cause some subtle issues.

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

It's too late to check the clip pointer for null in this condition as it was already dereferenced in the previous line.

A few other unsafe fragments:

• 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);
  ....
}

One of the developers seems to have suspected that this code didn't make sense, but they decided to comment on it instead of fixing 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;
 }
 ....
}

This condition is redundant and can be reduced to:

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

One more example:

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

Conclusion

These defects will hardly ever bother the eventual listeners but they may cause much trouble to Audacity users.

Other music software reviews:

• Part 1. MuseScore
• Part 2. Audacity
• Part 3. Rosegarden
• Part 4. Ardour
• Part 5. Steinberg SDKs

If you want to suggest some interesting audio software for us to review, feel free to email me.

It's very easy to try PVS-Studio analyzer with your project - just visit the download page.

#Cpp

0

0

0

0

SHARE

Tags:

#Cpp

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