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!
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.
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:
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.
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) constIt'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.
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".
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.
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.
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:
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 > 1One more example:
These defects will hardly ever bother the eventual listeners but they may cause much trouble to Audacity users.
Other music software reviews:
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.