From 0c8bedc59a65768100de93343e23f1e2de786746 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Sat, 24 Dec 2016 08:57:27 -0500 Subject: [PATCH] Exception safety in: subclasses of ExportPlugin; and more error checking --- src/FileIO.h | 2 ++ src/export/ExportCL.cpp | 22 +++++++++---- src/export/ExportFFmpeg.cpp | 20 +++++++---- src/export/ExportFLAC.cpp | 66 ++++++++++++++++++++++--------------- src/export/ExportMP2.cpp | 16 +++++---- src/export/ExportMP3.cpp | 43 +++++++++++++----------- src/export/ExportOGG.cpp | 39 +++++++++++++--------- src/export/ExportPCM.cpp | 43 +++++++++++++++--------- 8 files changed, 156 insertions(+), 95 deletions(-) diff --git a/src/FileIO.h b/src/FileIO.h index b4967513e..5200503df 100644 --- a/src/FileIO.h +++ b/src/FileIO.h @@ -26,6 +26,8 @@ class FileIO public: FileIO(const wxString & name, FileIOMode mode); + + // Calls Close() ~FileIO(); bool IsOpened(); diff --git a/src/export/ExportCL.cpp b/src/export/ExportCL.cpp index 2132daf51..e1c41230c 100644 --- a/src/export/ExportCL.cpp +++ b/src/export/ExportCL.cpp @@ -354,14 +354,19 @@ ProgressResult ExportCL::Export(AudacityProject *project, // Kick off the command ExportCLProcess process(&output); - rc = wxExecute(cmd, wxEXEC_ASYNC, &process); + { #if defined(__WXMSW__) - if (!opath.IsEmpty()) { - wxSetEnv(wxT("PATH"),opath.c_str()); - } + auto cleanup = finally( [&] { + if (!opath.IsEmpty()) { + wxSetEnv(wxT("PATH"),opath.c_str()); + } + } ); #endif + rc = wxExecute(cmd, wxEXEC_ASYNC, &process); + } + if (!rc) { wxMessageBox(wxString::Format(_("Cannot export audio to %s"), fName.c_str())); @@ -435,6 +440,11 @@ ProgressResult ExportCL::Export(AudacityProject *project, auto updateResult = ProgressResult::Success; { + auto closeIt = finally ( [&] { + // Should make the process die, before propagating any exception + process.CloseOutput(); + } ); + // Prepare the progress display ProgressDialog progress(_("Export"), selectionOnly ? @@ -475,6 +485,7 @@ ProgressResult ExportCL::Export(AudacityProject *project, while (bytes > 0) { os->Write(mixed, bytes); if (!os->IsOk()) { + updateResult = ProgressResult::Cancelled; break; } bytes -= os->LastWrite(); @@ -487,9 +498,6 @@ ProgressResult ExportCL::Export(AudacityProject *project, // Done with the progress display } - // Should make the process die - process.CloseOutput(); - // Wait for process to terminate while (process.IsActive()) { wxMilliSleep(10); diff --git a/src/export/ExportFFmpeg.cpp b/src/export/ExportFFmpeg.cpp index 9ec87dd37..9b215a3bf 100644 --- a/src/export/ExportFFmpeg.cpp +++ b/src/export/ExportFFmpeg.cpp @@ -165,7 +165,7 @@ private: unsigned mChannels{}; bool mSupportsUTF8{}; - // Smart pointer fields, their order is the reverse in which they are reset in Finalize(): + // Smart pointer fields, their order is the reverse in which they are reset in FreeResources(): AVFifoBufferHolder mEncAudioFifo; // FIFO to write incoming audio samples into AVMallocHolder mEncAudioFifoOutBuf; // buffer to read _out_ of the FIFO into AVFormatContextHolder mEncFormatCtx; // libavformat's context for our output file @@ -357,7 +357,7 @@ bool ExportFFmpeg::Init(const char *shortname, AudacityProject *project, const T return false; } - // Only now, we can keep all the resources until Finalize(). + // Only now, we can keep all the resources until after Finalize(). // Cancel the local cleanup. cleanup.release(); @@ -737,7 +737,6 @@ bool ExportFFmpeg::Finalize() // Write any file trailers. av_write_trailer(mEncFormatCtx.get()); - FreeResources(); return true; } @@ -774,7 +773,8 @@ bool ExportFFmpeg::EncodeAudioFrame(int16_t *pFrame, size_t frameSize) // Put the raw audio samples into the FIFO. ret = av_fifo_generic_write(mEncAudioFifo.get(), pRawSamples, nBytesToWrite,NULL); - wxASSERT(ret == nBytesToWrite); + if(ret != nBytesToWrite) + return false; if (nAudioFrameSizeOut > mEncAudioFifoOutBufSiz) { wxMessageBox(wxString::Format(_("FFmpeg : ERROR - nAudioFrameSizeOut too large.")), @@ -853,11 +853,13 @@ ProgressResult ExportFFmpeg::Export(AudacityProject *project, if (mSubFormat == FMT_OTHER) shortname = gPrefs->Read(wxT("/FileFormats/FFmpegFormat"),wxT("matroska")); ret = Init(shortname.mb_str(),project, metadata, subformat); + auto cleanup = finally ( [&] { FreeResources(); } ); if (!ret) return ProgressResult::Cancelled; size_t pcmBufferSize = 1024; + const WaveTrackConstArray waveTracks = tracks->GetWaveTrackConstArray(selectionOnly, false); auto mixer = CreateMixer(waveTracks, @@ -881,13 +883,19 @@ ProgressResult ExportFFmpeg::Export(AudacityProject *project, short *pcmBuffer = (short *)mixer->GetBuffer(); - EncodeAudioFrame(pcmBuffer, (pcmNumSamples)*sizeof(int16_t)*mChannels); + if (!EncodeAudioFrame( + pcmBuffer, (pcmNumSamples)*sizeof(int16_t)*mChannels)) { + updateResult = ProgressResult::Cancelled; + break; + } updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); } } - Finalize(); + if ( updateResult != ProgressResult::Cancelled ) + if ( !Finalize() ) + return ProgressResult::Cancelled; return updateResult; } diff --git a/src/export/ExportFLAC.cpp b/src/export/ExportFLAC.cpp index 2f427d92c..b3d376990 100644 --- a/src/export/ExportFLAC.cpp +++ b/src/export/ExportFLAC.cpp @@ -204,6 +204,7 @@ private: bool GetMetadata(AudacityProject *project, const Tags *tags); + // Should this be a stack variable instead in Export? FLAC__StreamMetadataHandle mMetadata; }; @@ -262,6 +263,10 @@ ProgressResult ExportFLAC::Export(AudacityProject *project, encoder.set_metadata(&p, 1); } + auto cleanup1 = finally( [&] { + mMetadata.reset(); // need this? + } ); + sampleFormat format; if (bitDepthPref == wxT("24")) { format = int24Sample; @@ -312,6 +317,13 @@ ProgressResult ExportFLAC::Export(AudacityProject *project, mMetadata.reset(); + auto cleanup2 = finally( [&] { +#ifndef LEGACY_FLAC + f.Detach(); // libflac closes the file +#endif + encoder.finish(); + } ); + const WaveTrackConstArray waveTracks = tracks->GetWaveTrackConstArray(selectionOnly, false); auto mixer = CreateMixer(waveTracks, @@ -322,38 +334,40 @@ ProgressResult ExportFLAC::Export(AudacityProject *project, ArraysOf tmpsmplbuf{ numChannels, SAMPLES_PER_RUN, true }; - { - ProgressDialog progress(wxFileName(fName).GetName(), - selectionOnly ? - _("Exporting the selected audio as FLAC") : - _("Exporting the entire project as FLAC")); + ProgressDialog progress(wxFileName(fName).GetName(), + selectionOnly ? + _("Exporting the selected audio as FLAC") : + _("Exporting the entire project as FLAC")); - while (updateResult == ProgressResult::Success) { - auto samplesThisRun = mixer->Process(SAMPLES_PER_RUN); - if (samplesThisRun == 0) { //stop encoding - break; - } - else { - for (size_t i = 0; i < numChannels; i++) { - samplePtr mixed = mixer->GetBuffer(i); - if (format == int24Sample) { - for (decltype(samplesThisRun) j = 0; j < samplesThisRun; j++) { - tmpsmplbuf[i][j] = ((int *)mixed)[j]; - } - } - else { - for (decltype(samplesThisRun) j = 0; j < samplesThisRun; j++) { - tmpsmplbuf[i][j] = ((short *)mixed)[j]; - } + while (updateResult == ProgressResult::Success) { + auto samplesThisRun = mixer->Process(SAMPLES_PER_RUN); + if (samplesThisRun == 0) { //stop encoding + break; + } + else { + for (size_t i = 0; i < numChannels; i++) { + samplePtr mixed = mixer->GetBuffer(i); + if (format == int24Sample) { + for (decltype(samplesThisRun) j = 0; j < samplesThisRun; j++) { + tmpsmplbuf[i][j] = ((int *)mixed)[j]; + } + } + else { + for (decltype(samplesThisRun) j = 0; j < samplesThisRun; j++) { + tmpsmplbuf[i][j] = ((short *)mixed)[j]; } } - encoder.process(reinterpret_cast(tmpsmplbuf.get()), samplesThisRun); } - updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); + if (! encoder.process( + reinterpret_cast( tmpsmplbuf.get() ), + samplesThisRun) ) { + updateResult = ProgressResult::Cancelled; + break; + } } - f.Detach(); // libflac closes the file - encoder.finish(); } + if (updateResult == ProgressResult::Success) + updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); return updateResult; } diff --git a/src/export/ExportMP2.cpp b/src/export/ExportMP2.cpp index 1de353290..1bc651f0c 100644 --- a/src/export/ExportMP2.cpp +++ b/src/export/ExportMP2.cpp @@ -217,6 +217,7 @@ ProgressResult ExportMP2::Export(AudacityProject *project, twolame_options *encodeOptions; encodeOptions = twolame_init(); + auto cleanup = finally( [&] { twolame_close(&encodeOptions); } ); twolame_set_in_samplerate(encodeOptions, (int)(rate + 0.5)); twolame_set_out_samplerate(encodeOptions, (int)(rate + 0.5)); @@ -227,7 +228,6 @@ ProgressResult ExportMP2::Export(AudacityProject *project, { wxMessageBox(_("Cannot export MP2 with this sample rate and bit rate"), _("Error"), wxICON_STOP); - twolame_close(&encodeOptions); return ProgressResult::Cancelled; } @@ -238,7 +238,6 @@ ProgressResult ExportMP2::Export(AudacityProject *project, FileIO outFile(fName, FileIO::Output); if (!outFile.IsOpened()) { wxMessageBox(_("Unable to open target file for writing")); - twolame_close(&encodeOptions); return ProgressResult::Cancelled; } @@ -288,6 +287,11 @@ ProgressResult ExportMP2::Export(AudacityProject *project, mp2Buffer.get(), mp2BufferSize); + if (mp2BufferNumBytes < 0) { + updateResult = ProgressResult::Cancelled; + break; + } + outFile.Write(mp2Buffer.get(), mp2BufferNumBytes); updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); @@ -302,15 +306,11 @@ ProgressResult ExportMP2::Export(AudacityProject *project, if (mp2BufferNumBytes > 0) outFile.Write(mp2Buffer.get(), mp2BufferNumBytes); - twolame_close(&encodeOptions); - /* Write ID3 tag if it was supposed to be at the end of the file */ if (id3len && endOfFile) outFile.Write(id3buffer.get(), id3len); - /* Close file */ - outFile.Close(); return updateResult; @@ -328,7 +328,9 @@ struct id3_tag_deleter { using id3_tag_holder = std::unique_ptr; // returns buffer len; caller frees -int ExportMP2::AddTags(AudacityProject * WXUNUSED(project), ArrayOf &buffer, bool *endOfFile, const Tags *tags) +int ExportMP2::AddTags( + AudacityProject * WXUNUSED(project), ArrayOf< char > &buffer, + bool *endOfFile, const Tags *tags) { #ifdef USE_LIBID3TAG id3_tag_holder tp { id3_tag_new() }; diff --git a/src/export/ExportMP3.cpp b/src/export/ExportMP3.cpp index a51047f44..64cc96bb2 100644 --- a/src/export/ExportMP3.cpp +++ b/src/export/ExportMP3.cpp @@ -1809,6 +1809,9 @@ ProgressResult ExportMP3::Export(AudacityProject *project, long bytes; size_t bufferSize = std::max(0, exporter.GetOutBufferSize()); + if (bufferSize == 0) + return ProgressResult::Cancelled; + ArrayOf buffer{ bufferSize }; wxASSERT(buffer); @@ -1873,6 +1876,7 @@ ProgressResult ExportMP3::Export(AudacityProject *project, wxString msg; msg.Printf(_("Error %ld returned from MP3 encoder"), bytes); wxMessageBox(msg); + updateResult = ProgressResult::Cancelled; break; } @@ -1882,28 +1886,29 @@ ProgressResult ExportMP3::Export(AudacityProject *project, } } - bytes = exporter.FinishStream(buffer.get()); + if ( updateResult != ProgressResult::Cancelled ) { + bytes = exporter.FinishStream(buffer.get()); - if (bytes) { - outFile.Write(buffer.get(), bytes); + if (bytes > 0) { + outFile.Write(buffer.get(), bytes); + } + + // Write ID3 tag if it was supposed to be at the end of the file + if (id3len > 0 && endOfFile) { + outFile.Write(id3buffer.get(), id3len); + } + + // Always write the info (Xing/Lame) tag. Until we stop supporting Lame + // versions before 3.98, we must do this after the MP3 file has been + // closed. + // + // Also, if beWriteInfoTag() is used, mGF will no longer be valid after + // this call, so do not use it. + exporter.PutInfoTag(outFile, pos); + + outFile.Close(); } - // Write ID3 tag if it was supposed to be at the end of the file - if (id3len && endOfFile) { - outFile.Write(id3buffer.get(), id3len); - } - - // Always write the info (Xing/Lame) tag. Until we stop supporting Lame - // versions before 3.98, we must do this after the MP3 file has been - // closed. - // - // Also, if beWriteInfoTag() is used, mGF will no longer be valid after - // this call, so do not use it. - exporter.PutInfoTag(outFile, pos); - - // Close the file - outFile.Close(); - return updateResult; } diff --git a/src/export/ExportOGG.cpp b/src/export/ExportOGG.cpp index 838b2486f..12f6e951e 100644 --- a/src/export/ExportOGG.cpp +++ b/src/export/ExportOGG.cpp @@ -193,6 +193,15 @@ ProgressResult ExportOGG::Export(AudacityProject *project, vorbis_dsp_state dsp; vorbis_block block; + auto cleanup = finally( [&] { + ogg_stream_clear(&stream); + + vorbis_block_clear(&block); + vorbis_dsp_clear(&dsp); + vorbis_info_clear(&info); + vorbis_comment_clear(&comment); + } ); + // Encoding setup vorbis_info_init(&info); vorbis_encode_init_vbr(&info, numChannels, (int)(rate + 0.5), quality); @@ -257,9 +266,10 @@ ProgressResult ExportOGG::Export(AudacityProject *project, float **vorbis_buffer = vorbis_analysis_buffer(&dsp, SAMPLES_PER_RUN); auto samplesThisRun = mixer->Process(SAMPLES_PER_RUN); + int err; if (samplesThisRun == 0) { // Tell the library that we wrote 0 bytes - signalling the end. - vorbis_analysis_wrote(&dsp, 0); + err = vorbis_analysis_wrote(&dsp, 0); } else { @@ -269,7 +279,7 @@ ProgressResult ExportOGG::Export(AudacityProject *project, } // tell the encoder how many samples we have - vorbis_analysis_wrote(&dsp, samplesThisRun); + err = vorbis_analysis_wrote(&dsp, samplesThisRun); } // I don't understand what this call does, so here is the comment @@ -278,22 +288,23 @@ ProgressResult ExportOGG::Export(AudacityProject *project, // vorbis does some data preanalysis, then divvies up blocks // for more involved (potentially parallel) processing. Get // a single block for encoding now - while (vorbis_analysis_blockout(&dsp, &block) == 1) { + while (!err && vorbis_analysis_blockout(&dsp, &block) == 1) { // analysis, assume we want to use bitrate management - vorbis_analysis(&block, NULL); - vorbis_bitrate_addblock(&block); + err = vorbis_analysis(&block, NULL); + if (!err) + err = vorbis_bitrate_addblock(&block); - while (vorbis_bitrate_flushpacket(&dsp, &packet)) { + while (!err && vorbis_bitrate_flushpacket(&dsp, &packet)) { // add the packet to the bitstream - ogg_stream_packetin(&stream, &packet); + err = ogg_stream_packetin(&stream, &packet); // From vorbis-tools-1.0/oggenc/encode.c: // If we've gone over a page boundary, we can do actual output, // so do so (for however many pages are available). - while (!eos) { + while (!err && !eos) { int result = ogg_stream_pageout(&stream, &page); if (!result) { break; @@ -309,17 +320,15 @@ ProgressResult ExportOGG::Export(AudacityProject *project, } } + if (err) { + updateResult = ProgressResult::Cancelled; + break; + } + updateResult = progress.Update(mixer->MixGetCurrentTime() - t0, t1 - t0); } } - ogg_stream_clear(&stream); - - vorbis_block_clear(&block); - vorbis_dsp_clear(&dsp); - vorbis_info_clear(&info); - vorbis_comment_clear(&comment); - outFile.Close(); return updateResult; diff --git a/src/export/ExportPCM.cpp b/src/export/ExportPCM.cpp index e9df03804..5fae06093 100644 --- a/src/export/ExportPCM.cpp +++ b/src/export/ExportPCM.cpp @@ -331,7 +331,7 @@ private: ArrayOf AdjustString(const wxString & wxStr, int sf_format); bool AddStrings(AudacityProject *project, SNDFILE *sf, const Tags *tags, int sf_format); - void AddID3Chunk(wxString fName, const Tags *tags, int sf_format); + bool AddID3Chunk(wxString fName, const Tags *tags, int sf_format); }; @@ -516,6 +516,7 @@ ProgressResult ExportPCM::Export(AudacityProject *project, _("Error while writing %s file (disk full?).\nLibsndfile says \"%s\""), formatStr.c_str(), wxString::FromAscii(buffer2).c_str())); + updateResult = ProgressResult::Cancelled; break; } @@ -534,7 +535,8 @@ ProgressResult ExportPCM::Export(AudacityProject *project, if (((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_AIFF) || ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAV)) - AddID3Chunk(fName, metadata, sf_format); + if (!AddID3Chunk(fName, metadata, sf_format) ) + return ProgressResult::Cancelled; return updateResult; } @@ -699,7 +701,7 @@ struct id3_tag_deleter { }; using id3_tag_holder = std::unique_ptr; -void ExportPCM::AddID3Chunk(wxString fName, const Tags *tags, int sf_format) +bool ExportPCM::AddID3Chunk(wxString fName, const Tags *tags, int sf_format) { #ifdef USE_LIBID3TAG id3_tag_holder tp { id3_tag_new() }; @@ -784,12 +786,12 @@ void ExportPCM::AddID3Chunk(wxString fName, const Tags *tags, int sf_format) len = id3_tag_render(tp.get(), 0); if (len == 0) - return; + return true; if ((len % 2) != 0) len++; // Length must be even. ArrayOf buffer { len, true }; if (buffer == NULL) - return; + return false; // Zero all locations, for ending odd UTF16 content // correctly, i.e., two '\0's at the end. @@ -797,33 +799,44 @@ void ExportPCM::AddID3Chunk(wxString fName, const Tags *tags, int sf_format) id3_tag_render(tp.get(), buffer.get()); wxFFile f(fName, wxT("r+b")); - // FIXME: TRAP_ERR wxFFILE ops in Export PCM ID3 could fail. if (f.IsOpened()) { wxUint32 sz; sz = (wxUint32) len; - f.SeekEnd(0); + if (!f.SeekEnd(0)) + return false; if ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_WAV) - f.Write("id3 ", 4); // Must be lower case for foobar2000. + { + if (4 != f.Write("id3 ", 4))// Must be lower case for foobar2000. + return false ; + } else { - f.Write("ID3 ", 4); + if (4 != f.Write("ID3 ", 4)) + return false; sz = wxUINT32_SWAP_ON_LE(sz); } - f.Write(&sz, 4); + if (4 != f.Write(&sz, 4)) + return false; - f.Write(buffer.get(), len); + if (len != f.Write(buffer.get(), len)) + return false; sz = (wxUint32) f.Tell() - 8; if ((sf_format & SF_FORMAT_TYPEMASK) == SF_FORMAT_AIFF) sz = wxUINT32_SWAP_ON_LE(sz); - f.Seek(4); - f.Write(&sz, 4); + if (!f.Seek(4)) + return false; + if (4 != f.Write(&sz, 4)) + return false; - f.Close(); + if (!f.Close()) + return false; } + else + return false; #endif - return; + return true; } wxWindow *ExportPCM::OptionsCreate(wxWindow *parent, int format)