Review of music software code defects. Part 2. Audacity

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

Introduction

Audacity is a free multi-platform audio editor for audio files, 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, which contained about a thousand files with the source code of different libraries, was excluded from the analysis. The article includes only the most interesting errors. To view the full report, authors can verify the project themselves by requesting a temporary key in support.

PVS-Studio is a tool for detecting errors in the source code of programs written in C, C ++ and C # languages. Works in the environment of Windows and Linux.
')

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

The above code snippet is an ideal candidate 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.

A few 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]". 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 transferred to it. Most likely, they wanted to get the difference between adjacent elements of the buffer, but forgot to change the two by one after copying a fragment of the string.

Misuse of functions

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

Incorrect use of the std :: remove () function is so common that such an example is given in the documentation for this diagnostic. Therefore, in order not to copy the description from the documentation again, I will simply provide 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 the function 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 is the prototype of the Left () function:

 wxString Left (size_t count) const 

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

A nightmare for PC users

V590 Consider inspecting this expression. The expression is misprint. ExtImportPrefs.cpp 600

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

Many users of computer programs ever clicked "not there" and tried to cancel 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 you clicked on "No" or not: D

Here is the truth table 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, it will be clearer to rewrite to use the if operator.

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

Very strange code detected by the analyzer. 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<Effect *>(instance) }; } 

An example of a very interesting use of unique_ptr . This one-liner (without formatting) serves to create unique_ptr and immediately destroy it, freeing the instance pointer.

miscellanea

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 an 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 doing a negative number shift, which can lead to unobvious problems.

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 by the line above.

Some more dangerous places:

• V595 The 'outputMeterFloats' pointer was used before it was verified against nullptr. Check lines: 5246, 5255. AudioIO.cpp 5246
• V595 The 'buffer2' pointer was used before it was verified nullptr. Check lines: 404, 409. Compressor.cpp 404
• V595 The 'p' pointer was used against it. Check lines: 946, 974. ControlToolBar.cpp 946
• V595 'mParent ’pointer was used 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 code does not make sense, but instead of correcting the code, I decided to comment on it.

V728 Anonymous check can be simplified. The '||' "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 one more example of such code:

• V728 Anonymous check can be simplified. The '||' "j-> hasFixedBinCount 'and' j-> hasFixedBinCount '. LoadVamp.cpp 297

Conclusion

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

Other reviews:

• Review of music software code defects. Part 1. MuseScore
• Review of music software code defects. 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 .

It is very easy to try the PVS-Studio analyzer on your project, 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