From 5f5b9778de8c6c7c4e10b63b49d3bfa0a3aa7ef1 Mon Sep 17 00:00:00 2001 From: "v.audacity" Date: Wed, 8 Feb 2012 05:09:14 +0000 Subject: [PATCH] Fixes per Vigilant Sentry (http://www.vigilantsw.com/) * Fix memory leaks. * Add comments about initializations and checking for successful results. * Add checks for NULL deref. * Consistency in "TODO" vs "TO-DO" comments! --- src/AudioIO.cpp | 8 ++++++++ src/AutoRecovery.cpp | 3 ++- src/DeviceManager.cpp | 3 +-- src/FreqWindow.cpp | 8 ++++---- src/LabelTrack.cpp | 2 ++ src/Menus.cpp | 4 ++++ src/Sequence.cpp | 13 +++++++++---- src/WaveClip.cpp | 2 +- src/blockfile/ODDecodeBlockFile.cpp | 2 +- src/blockfile/SimpleBlockFile.cpp | 2 +- src/effects/AutoDuck.cpp | 2 ++ src/effects/Equalization.cpp | 14 ++++++++++++-- src/effects/vamp/VampEffect.cpp | 2 +- src/export/Export.cpp | 3 +-- src/import/Import.cpp | 6 +++++- src/import/ImportFLAC.cpp | 17 ++++++++++------- src/import/ImportMIDI.cpp | 1 + src/ondemand/ODWaveTrackTaskQueue.cpp | 2 ++ src/prefs/KeyConfigPrefs.cpp | 6 ++++++ src/prefs/PrefsDialog.cpp | 2 +- src/toolbars/ControlToolBar.cpp | 7 +++---- src/toolbars/DeviceToolBar.cpp | 2 +- src/widgets/ErrorDialog.cpp | 6 +++++- 23 files changed, 83 insertions(+), 34 deletions(-) diff --git a/src/AudioIO.cpp b/src/AudioIO.cpp index 2201f456d..ebb89aa48 100644 --- a/src/AudioIO.cpp +++ b/src/AudioIO.cpp @@ -978,7 +978,10 @@ bool AudioIO::StartPortAudioStream(double sampleRate, playbackDeviceInfo = Pa_GetDeviceInfo( playbackParameters->device ); if( playbackDeviceInfo == NULL ) + { + delete playbackParameters; return false; + } // regardless of source formats, we always mix to float playbackParameters->sampleFormat = paFloat32; @@ -1005,7 +1008,11 @@ bool AudioIO::StartPortAudioStream(double sampleRate, captureDeviceInfo = Pa_GetDeviceInfo( captureParameters->device ); if( captureDeviceInfo == NULL ) + { + delete captureParameters; + delete playbackParameters; return false; + } captureParameters->sampleFormat = AudacityToPortAudioSampleFormat(mCaptureFormat); @@ -1087,6 +1094,7 @@ void AudioIO::StartMonitoring(double sampleRate) success = StartPortAudioStream(sampleRate, (unsigned int)playbackChannels, (unsigned int)captureChannels, captureFormat); + // TODO: Check return value of success. // Now start the PortAudio stream! mLastPaError = Pa_StartStream( mPortStreamV19 ); diff --git a/src/AutoRecovery.cpp b/src/AutoRecovery.cpp index 7ac329584..9dd519d76 100644 --- a/src/AutoRecovery.cpp +++ b/src/AutoRecovery.cpp @@ -220,7 +220,8 @@ static bool RecoverAllProjects(AudacityProject** pproj) bool ShowAutoRecoveryDialogIfNeeded(AudacityProject** pproj, bool *didRecoverAnything) { - *didRecoverAnything = false; + if (didRecoverAnything) + *didRecoverAnything = false; if (HaveFilesToRecover()) { AutoRecoveryDialog dlg(*pproj); diff --git a/src/DeviceManager.cpp b/src/DeviceManager.cpp index 284b3007f..4cbcd33ab 100644 --- a/src/DeviceManager.cpp +++ b/src/DeviceManager.cpp @@ -270,12 +270,11 @@ void DeviceManager::Rescan() } int nDevices = Pa_GetDeviceCount(); - int i; //The heirarchy for devices is Host/device/source. //Some newer systems aggregate this. //So we need to call port mixer for every device to get the sources - for (i = 0; i < nDevices; i++) { + for (int i = 0; i < nDevices; i++) { const PaDeviceInfo *info = Pa_GetDeviceInfo(i); if (info->maxOutputChannels > 0) { AddSources(i, info->defaultSampleRate, &mOutputDeviceSourceMaps, 0); diff --git a/src/FreqWindow.cpp b/src/FreqWindow.cpp index fa96191d6..d6e8561e8 100644 --- a/src/FreqWindow.cpp +++ b/src/FreqWindow.cpp @@ -524,7 +524,7 @@ void FreqWindow::DrawPlot() xMax = mRate / 2; xRatio = xMax / xMin; xPos = xMin; - xLast = xPos / 2.0; + xLast = xPos / 2.0; // ANSWER-ME: Vigilant Sentry notes this var is unused after this assignment. Delete it and the var decl? if (mLogAxis) { xStep = pow(2.0f, (log(xRatio) / log(2.0f)) / width); @@ -540,7 +540,7 @@ void FreqWindow::DrawPlot() xMin = 0; xMax = mProcessedSize / mRate; xPos = xMin; - xLast = xPos / 2.0; + xLast = xPos / 2.0; // ANSWER-ME: Vigilant Sentry notes this var is unused after this assignment. Delete it and the var decl? xStep = (xMax - xMin) / width; hRuler->ruler.SetLog(false); hRuler->ruler.SetUnits(_("s")); @@ -787,7 +787,7 @@ void FreqWindow::PlotPaint(wxPaintEvent & evt) xMax = mRate / 2; xRatio = xMax / xMin; xPos = xMin; - xLast = xPos / 2.0; + xLast = xPos / 2.0; // ANSWER-ME: Vigilant Sentry notes this var is unused after this assignment. Delete it and the var decl? if (mLogAxis) xStep = pow(2.0f, (log(xRatio) / log(2.0f)) / width); else @@ -796,7 +796,7 @@ void FreqWindow::PlotPaint(wxPaintEvent & evt) xMin = 0; xMax = mProcessedSize / mRate; xPos = xMin; - xLast = xPos / 2.0; + xLast = xPos / 2.0; // ANSWER-ME: Vigilant Sentry notes this var is unused after this assignment. Delete it and the var decl? xStep = (xMax - xMin) / width; } diff --git a/src/LabelTrack.cpp b/src/LabelTrack.cpp index 9651716f2..3ff0cac5f 100644 --- a/src/LabelTrack.cpp +++ b/src/LabelTrack.cpp @@ -1220,6 +1220,8 @@ void LabelStruct::AdjustEdge( int iEdge, double fNewTime) // We're moving the label. Adjust both left and right edge. void LabelStruct::MoveLabel( int iEdge, double fNewTime) { + // ANSWER-ME: Vigilant Sentry notes this "width" shadows the member var of same name. + // Is that what we actually want? double width = getDuration(); if( iEdge < 0 ) diff --git a/src/Menus.cpp b/src/Menus.cpp index 70e3c98cf..7f6fdaa6c 100644 --- a/src/Menus.cpp +++ b/src/Menus.cpp @@ -5573,6 +5573,8 @@ void AudacityProject::OnExportCleanSpeechPresets() int lenPreset = sizeof(preset); int count = presetsFile.Write(preset, lenPreset); + // ANSWER-ME: Vigilant Sentry notes "count" is unused after this assignment. + // Should be checked against expectedCount -- but CleanSpeech is going away!!! ;-) count = presetsFile.Write(pNoiseGate, expectedCount); presetsFile.Close(); @@ -5628,6 +5630,8 @@ void AudacityProject::OnImportCleanSpeechPresets() } int expectedCount = wxGetApp().GetCleanSpeechNoiseGateExpectedCount(); float* pNoiseGate = wxGetApp().GetCleanSpeechNoiseGate(); + // ANSWER-ME: Vigilant Sentry notes "count" is unused after this assignment. + // Should be checked against expectedCount -- but CleanSpeech is going away!!! ;-) count = presetsFile.Read(pNoiseGate, expectedCount); gPrefs->Write(wxT("/CsPresets/ClickThresholdLevel"), preset[2]); diff --git a/src/Sequence.cpp b/src/Sequence.cpp index ddf40db8e..7da07d462 100644 --- a/src/Sequence.cpp +++ b/src/Sequence.cpp @@ -181,10 +181,10 @@ bool Sequence::ConvertToSampleFormat(sampleFormat format, bool* pbChanged) if (pOldBlockFile->IsAlias()) { // No conversion of aliased data. - //vvvvv Should the user be alerted, as we're not actually converting the aliased file? + //v Should the user be alerted, as we're not actually converting the aliased file? // James (2011-12-01, offlist) believes this is okay because we are assuring // the user we'll do the format conversion if we turn this into a non-aliased block. - // TO-DO: Confirm that. + // TODO: Confirm that. pNewBlockArray->Add(pOldSeqBlock); } else @@ -511,6 +511,7 @@ bool Sequence::Paste(sampleCount s, const Sequence *src) return ConsistencyCheck(wxT("Paste branch one")); } + // FIX-ME: "b" is unsigned, so it's pointless to check that it's >= 0. if (b >= 0 && b < numBlocks && ((mBlock->Item(b)->f->GetLength() + addedLen) < mMaxSamples)) { // Special case: we can fit all of the new samples inside of @@ -626,8 +627,10 @@ bool Sequence::Paste(sampleCount s, const Sequence *src) insertBlock->f = mDirManager->CopyBlockFile(srcBlock->Item(i)->f); if (!insertBlock->f) { - // TODO error: Could not paste! (Out of disk space?) - wxASSERT(false); + wxASSERT(false); // TODO: Handle this better, alert the user of failure. + delete insertBlock; + newBlock->Clear(); + delete newBlock; return false; } @@ -772,6 +775,8 @@ bool Sequence::AppendBlock(SeqBlock * b) newBlock->f = mDirManager->CopyBlockFile(b->f); if (!newBlock->f) { /// \todo Error Could not paste! (Out of disk space?) + wxASSERT(false); // TODO: Handle this better, alert the user of failure. + delete newBlock; return false; } diff --git a/src/WaveClip.cpp b/src/WaveClip.cpp index 601337986..cd67835c7 100644 --- a/src/WaveClip.cpp +++ b/src/WaveClip.cpp @@ -997,7 +997,7 @@ void WaveClip::ConvertToSampleFormat(sampleFormat format) bool bResult = mSequence->ConvertToSampleFormat(format, &bChanged); if (bResult && bChanged) MarkChanged(); - wxASSERT(bResult); // TO-DO: Throw an actual error. + wxASSERT(bResult); // TODO: Throw an actual error. } void WaveClip::UpdateEnvelopeTrackLen() diff --git a/src/blockfile/ODDecodeBlockFile.cpp b/src/blockfile/ODDecodeBlockFile.cpp index 9ecb91d19..3bb6b0274 100644 --- a/src/blockfile/ODDecodeBlockFile.cpp +++ b/src/blockfile/ODDecodeBlockFile.cpp @@ -336,7 +336,7 @@ int ODDecodeBlockFile::WriteODDecodeBlockFile() mLen, mFormat, NULL);//summaryData); - wxASSERT(bSuccess); // TO-DO: Handle failure here by alert to user and undo partial op. + wxASSERT(bSuccess); // TODO: Handle failure here by alert to user and undo partial op. mFileNameMutex.Unlock(); diff --git a/src/blockfile/SimpleBlockFile.cpp b/src/blockfile/SimpleBlockFile.cpp index b3b9925a7..b8e95088d 100644 --- a/src/blockfile/SimpleBlockFile.cpp +++ b/src/blockfile/SimpleBlockFile.cpp @@ -111,7 +111,7 @@ SimpleBlockFile::SimpleBlockFile(wxFileName baseFileName, if (!(allowDeferredWrite && useCache) && !bypassCache) { bool bSuccess = WriteSimpleBlockFile(sampleData, sampleLen, format, NULL); - wxASSERT(bSuccess); // TO-DO: Handle failure here by alert to user and undo partial op. + wxASSERT(bSuccess); // TODO: Handle failure here by alert to user and undo partial op. } if (useCache) { diff --git a/src/effects/AutoDuck.cpp b/src/effects/AutoDuck.cpp index df9f9b61d..d7b970f72 100644 --- a/src/effects/AutoDuck.cpp +++ b/src/effects/AutoDuck.cpp @@ -436,6 +436,7 @@ bool EffectAutoDuck::ApplyDuckFade(int trackNumber, WaveTrack* t, } } + delete[] buf; return cancel; } @@ -954,6 +955,7 @@ void EffectAutoDuckPanel::OnMotion(wxMouseEvent &evt) else dist = abs(evt.GetX() - mMoveStartControlPoints[mCurrentControlPoint].x); + // TODO: Get rid of unused 'dist' var within this scope? float newValue; diff --git a/src/effects/Equalization.cpp b/src/effects/Equalization.cpp index 019230b4d..926c6c420 100644 --- a/src/effects/Equalization.cpp +++ b/src/effects/Equalization.cpp @@ -2056,7 +2056,11 @@ void EqualizationDialog::LayoutEQSliders() void EqualizationDialog::GraphicEQ(Envelope *env) { - double value, dist, span, s; + // Vigilant Sentry noted this "value" var had not previously been initialized, + // so there must be some path to lines 2133 and 2179 that do not get a setting. + // ANSWER-ME: Is this a good default value? + double value = 0.0; + double dist, span, s; env->Flatten(0.); env->SetTrackLen(1.0); @@ -2981,7 +2985,7 @@ void EditCurvesDialog::OnRename(wxCommandEvent &event) { wxString name; int numCurves = mEditCurves.GetCount(); - int curve; + int curve = 0; // Setup list of characters that aren't allowed wxArrayString exclude; @@ -3052,6 +3056,11 @@ void EditCurvesDialog::OnRename(wxCommandEvent &event) { if(overwrite) { // Overwrite another curve with 'unnamed' + // ANSWER-ME: What is the expected value of "curve" here? + // It was not previously initialized at declaration. + // And if we expect it to have been through the above loop, + // it will be numCurves, which is a bad index, right? + // I've initialized it to zero, so we at least know what value it has. mEditCurves[ curve ].Name = name; mList->SetItem(curve, 0, name); mEditCurves[ curve ].points = mEditCurves[ item ].points; @@ -3070,6 +3079,7 @@ void EditCurvesDialog::OnRename(wxCommandEvent &event) { if(overwrite) { // Overwrite another curve with this one, then delete this one + // ANSWER-ME: Same question as above, i.e., what's the expected value of "curve" here? mEditCurves[ curve ].Name = name; mEditCurves[ curve ].points = mEditCurves[ item ].points; mEditCurves.RemoveAt( item ); diff --git a/src/effects/vamp/VampEffect.cpp b/src/effects/vamp/VampEffect.cpp index 93cba68dc..fdfdeffee 100644 --- a/src/effects/vamp/VampEffect.cpp +++ b/src/effects/vamp/VampEffect.cpp @@ -221,7 +221,7 @@ bool VampEffect::Process() mTracks->Add(ltrack); - float **data = new float*[channels]; + float **data = new float*[channels]; // ANSWER-ME: Vigilant Sentry marks this as memory leak, var "data" not deleted. for (int c = 0; c < channels; ++c) data[c] = new float[block]; sampleCount originalLen = len; diff --git a/src/export/Export.cpp b/src/export/Export.cpp index 9f1b2d4ed..e5d8ef3eb 100644 --- a/src/export/Export.cpp +++ b/src/export/Export.cpp @@ -640,10 +640,9 @@ bool Exporter::GetFilename() // This causes problems for the exporter, so we don't allow it. // Overwritting non-missing aliased files is okay. // Also, this can only happen for uncompressed audio. - size_t i; bool overwritingMissingAlias; overwritingMissingAlias = false; - for (i = 0; i < gAudacityProjects.GetCount(); i++) { + for (size_t i = 0; i < gAudacityProjects.GetCount(); i++) { AliasedFileArray aliasedFiles; FindDependencies(gAudacityProjects[i], &aliasedFiles); size_t j; diff --git a/src/import/Import.cpp b/src/import/Import.cpp index 378df7828..16605a390 100644 --- a/src/import/Import.cpp +++ b/src/import/Import.cpp @@ -282,7 +282,11 @@ void Importer::WriteImportItems() gPrefs->Write (name, val); } /* If we had more items than we have now, delete the excess */ - for (i = this->mExtImportItems->Count(); i >= 0; i++) + // ANSWER-ME: Vigilant Sentry notes i is unsigned so (i >= 0) is always true. + // If that's what you want, why not make the condition just be "true"? Or should it be an int? + // But also, it looks like it's supposed to be a down-counting loop, so is i++ correct? + // Rather, shouldn't i be set to this->mExtImportItems->Count() each time? + for (i = this->mExtImportItems->Count(); i >= 0; i++) { name.Printf (wxT("/ExtImportItems/Item%d"), i); if (gPrefs->Read(name, &val)) diff --git a/src/import/ImportFLAC.cpp b/src/import/ImportFLAC.cpp index a3259b2e1..f2bcbf09c 100644 --- a/src/import/ImportFLAC.cpp +++ b/src/import/ImportFLAC.cpp @@ -467,13 +467,16 @@ int FLACImportFileHandle::Import(TrackFactory *trackFactory, useOD=true; #endif -#ifdef LEGACY_FLAC - bool res = (mFile->process_until_end_of_file() != 0); -#else - bool res = true; - if(!useOD) - res = (mFile->process_until_end_of_stream() != 0); -#endif + // ANSWER-ME: Vigilant Sentry: Variable res unused after assignment (error code DA1) + // Plus, it's not even declared in the LEGACY_FLAC clause + //#ifdef LEGACY_FLAC + // bool res = (mFile->process_until_end_of_file() != 0); + //#else + // bool res = true; + // if(!useOD) + // res = (mFile->process_until_end_of_stream() != 0); + //#endif + //add the task to the ODManager if(useOD) { diff --git a/src/import/ImportMIDI.cpp b/src/import/ImportMIDI.cpp index 6ee7a83c8..b0f825e1d 100644 --- a/src/import/ImportMIDI.cpp +++ b/src/import/ImportMIDI.cpp @@ -53,6 +53,7 @@ bool ImportMIDI(wxString fName, NoteTrack * dest) if(new_seq->get_read_error() == alg_error_open){ wxMessageBox( _("Could not open file ") + fName + wxT(".")); mf.Close(); + delete new_seq; return false; } diff --git a/src/ondemand/ODWaveTrackTaskQueue.cpp b/src/ondemand/ODWaveTrackTaskQueue.cpp index 03e4b7b1c..5a09c88b8 100644 --- a/src/ondemand/ODWaveTrackTaskQueue.cpp +++ b/src/ondemand/ODWaveTrackTaskQueue.cpp @@ -225,6 +225,7 @@ WaveTrack* ODWaveTrackTaskQueue::GetWaveTrack(size_t x) { WaveTrack* ret = NULL; mTracksMutex.Lock(); + // FIX-ME: x is unsigned so there's no point in checking that it's >= 0. if(x>=0&&x= 0. if(x>=0&&x= mNames.GetCount()) return; @@ -414,6 +416,8 @@ void KeyConfigPrefs::OnSet(wxCommandEvent & e) void KeyConfigPrefs::OnClear(wxCommandEvent& event) { mKey->Clear(); + // ANSWER-ME: mCommandSelected is unsigned, so there's no point checking whether it's < 0. + // Should it be a signed int instead or remove that check? if (mCommandSelected < 0 || mCommandSelected >= mNames.GetCount()) { return; } @@ -500,6 +504,8 @@ void KeyConfigPrefs::OnCategory(wxCommandEvent & e) void KeyConfigPrefs::OnItemSelected(wxListEvent & e) { mCommandSelected = e.GetIndex(); + // ANSWER-ME: mCommandSelected is unsigned, so there's no point checking whether it's < 0. + // Should it be a signed int instead or remove that check? if (mCommandSelected < 0 || mCommandSelected >= mNames.GetCount()) { mKey->SetLabel(wxT("")); return; diff --git a/src/prefs/PrefsDialog.cpp b/src/prefs/PrefsDialog.cpp index 3a5dae24a..9b543f1b2 100644 --- a/src/prefs/PrefsDialog.cpp +++ b/src/prefs/PrefsDialog.cpp @@ -249,7 +249,7 @@ void PrefsDialog::OnOK(wxCommandEvent & event) // set AudioIONotBusyFlag/AudioIOBusyFlag according to monitoring, as well. // Instead allow it because unlike recording, for example, monitoring // is not clearly something that should prohibit opening prefs. - // TO-DO: We *could* be smarter in this method and call HandleDeviceChange() + // TODO: We *could* be smarter in this method and call HandleDeviceChange() // only when the device choices actually changed. True of lots of prefs! // As is, we always stop monitoring before handling the device change. if (gAudioIO->IsMonitoring()) diff --git a/src/toolbars/ControlToolBar.cpp b/src/toolbars/ControlToolBar.cpp index e6eec7e48..f0a596b11 100644 --- a/src/toolbars/ControlToolBar.cpp +++ b/src/toolbars/ControlToolBar.cpp @@ -742,7 +742,8 @@ void ControlToolBar::StopPlaying(bool stopStream /* = true*/) void ControlToolBar::OnBatch(wxCommandEvent &evt) { AudacityProject *proj = GetActiveProject(); - proj->OnApplyChain(); + if (proj) + proj->OnApplyChain(); mPlay->Enable(); mStop->Enable(); @@ -762,8 +763,7 @@ void ControlToolBar::OnRecord(wxCommandEvent &evt) AudacityProject *p = GetActiveProject(); if (p && p->GetCleanSpeechMode()) { size_t numProjects = gAudacityProjects.Count(); - bool tracks = (p && !p->GetTracks()->IsEmpty()); - if (tracks || (numProjects > 1)) { + if (!p->GetTracks()->IsEmpty() || (numProjects > 1)) { wxMessageBox(_("Recording in CleanSpeech mode is not possible when a track, or more than one project, is already open."), _("Recording not permitted"), wxOK | wxICON_INFORMATION, @@ -817,7 +817,6 @@ void ControlToolBar::OnRecord(wxCommandEvent &evt) int recordingChannels = 0; bool shifted = mRecord->WasShiftDown(); if (shifted) { - TrackListIterator it(t); WaveTrack *wt; bool sel = false; double allt0 = t0; diff --git a/src/toolbars/DeviceToolBar.cpp b/src/toolbars/DeviceToolBar.cpp index 074cd664b..7772ab796 100644 --- a/src/toolbars/DeviceToolBar.cpp +++ b/src/toolbars/DeviceToolBar.cpp @@ -717,7 +717,7 @@ void DeviceToolBar::OnChoice(wxCommandEvent &event) // set AudioIONotBusyFlag/AudioIOBusyFlag according to monitoring, as well. // Instead allow it because unlike recording, for example, monitoring // is not clearly something that should prohibit changing device. - // TO-DO: We *could* be smarter in this method and call HandleDeviceChange() + // TODO: We *could* be smarter in this method and call HandleDeviceChange() // only when the device choices actually changed. True of lots of prefs! // As is, we always stop monitoring before handling the device change. if (gAudioIO->IsMonitoring()) diff --git a/src/widgets/ErrorDialog.cpp b/src/widgets/ErrorDialog.cpp index a09acb198..612fa25ea 100644 --- a/src/widgets/ErrorDialog.cpp +++ b/src/widgets/ErrorDialog.cpp @@ -301,6 +301,8 @@ void ShowModelessErrorDialog(wxWindow *parent, ErrorDialog *dlog = new ErrorDialog(parent, dlogTitle, message, helpURL, Close, false); dlog->CentreOnParent(); dlog->Show(); + // ANSWER-ME: Vigilant Sentry flags this method as not deleting dlog, so a mem leak. + // ANSWER-ME: This is unused. Delete it or are there plans for it? } void ShowAliasMissingDialog(AudacityProject *parent, @@ -321,8 +323,10 @@ void ShowAliasMissingDialog(AudacityProject *parent, point.y = 100; dlog->SetPosition(point); dlog->CentreOnParent(wxHORIZONTAL); - + dlog->Show(); + // ANSWER-ME: Vigilant Sentry flags this method as not deleting dlog, so a mem leak. + // ANSWER-ME: Why is this modeless? Shouldn't it require user action before proceeding? } /// Mostly we use this so that we have the code for resizability