From bfdf13dca365fb6ac9a5912d6a1a9efb08b2e361 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 13:36:22 +0200 Subject: [PATCH] decoder/Plugin: allow scan_{file,stream}() to throw Bug #915 is about an I/O exception thrown where none was allowed, leading to crash via std::terminate(). However, instead of catching and logging the error inside the decoder plugin, it should be able to propagate the I/O error to the MPD core, so MPD can avoid trying other decoder plugins. Closes https://github.com/MusicPlayerDaemon/MPD/issues/915 --- NEWS | 2 + src/SongUpdate.cxx | 41 +++++++++++++------ src/TagFile.cxx | 10 ++--- src/TagFile.hxx | 8 +++- src/TagStream.cxx | 16 +++----- src/TagStream.hxx | 18 ++++++-- src/decoder/DecoderPlugin.hxx | 20 +++++---- .../plugins/AudiofileDecoderPlugin.cxx | 2 +- src/decoder/plugins/DsdiffDecoderPlugin.cxx | 2 +- src/decoder/plugins/DsfDecoderPlugin.cxx | 2 +- src/decoder/plugins/FaadDecoderPlugin.cxx | 2 +- src/decoder/plugins/FfmpegDecoderPlugin.cxx | 5 +-- src/decoder/plugins/FlacDecoderPlugin.cxx | 8 ++-- src/decoder/plugins/MadDecoderPlugin.cxx | 2 +- src/decoder/plugins/MpcdecDecoderPlugin.cxx | 2 +- src/decoder/plugins/OpusDecoderPlugin.cxx | 2 +- src/decoder/plugins/SndfileDecoderPlugin.cxx | 2 +- src/decoder/plugins/VorbisDecoderPlugin.cxx | 2 +- src/decoder/plugins/WavpackDecoderPlugin.cxx | 2 +- 19 files changed, 90 insertions(+), 58 deletions(-) diff --git a/NEWS b/NEWS index bdc81cb38..d4e05e837 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ ver 0.21.25 (not yet released) * decoder - opus: apply pre-skip and end trimming - opus: fix memory leak + - opus: fix crash bug + - vorbis: fix crash bug * output - osx: improve sample rate selection - osx: fix noise while stopping diff --git a/src/SongUpdate.cxx b/src/SongUpdate.cxx index 0743216a7..fde991bfd 100644 --- a/src/SongUpdate.cxx +++ b/src/SongUpdate.cxx @@ -79,17 +79,22 @@ Song::UpdateFile(Storage &storage) noexcept TagBuilder tag_builder; auto new_audio_format = AudioFormat::Undefined(); - const auto path_fs = storage.MapFS(relative_uri.c_str()); - if (path_fs.IsNull()) { - const auto absolute_uri = - storage.MapUTF8(relative_uri.c_str()); - if (!tag_stream_scan(absolute_uri.c_str(), tag_builder, - &new_audio_format)) - return false; - } else { - if (!ScanFileTagsWithGeneric(path_fs, tag_builder, + try { + const auto path_fs = storage.MapFS(relative_uri.c_str()); + if (path_fs.IsNull()) { + const auto absolute_uri = + storage.MapUTF8(relative_uri.c_str()); + if (!tag_stream_scan(absolute_uri.c_str(), tag_builder, &new_audio_format)) - return false; + return false; + } else { + if (!ScanFileTagsWithGeneric(path_fs, tag_builder, + &new_audio_format)) + return false; + } + } catch (...) { + // TODO: log or propagate I/O errors? + return false; } mtime = info.mtime; @@ -153,8 +158,14 @@ DetachedSong::LoadFile(Path path) noexcept return false; TagBuilder tag_builder; - if (!ScanFileTagsWithGeneric(path, tag_builder)) + + try { + if (!ScanFileTagsWithGeneric(path, tag_builder)) + return false; + } catch (...) { + // TODO: log or propagate I/O errors? return false; + } mtime = fi.GetModificationTime(); tag_builder.Commit(tag); @@ -173,8 +184,14 @@ DetachedSong::Update() noexcept return LoadFile(path_fs); } else if (IsRemote()) { TagBuilder tag_builder; - if (!tag_stream_scan(uri.c_str(), tag_builder)) + + try { + if (!tag_stream_scan(uri.c_str(), tag_builder)) + return false; + } catch (...) { + // TODO: log or propagate I/O errors? return false; + } mtime = std::chrono::system_clock::time_point::min(); tag_builder.Commit(tag); diff --git a/src/TagFile.cxx b/src/TagFile.cxx index f30037481..0c57a6607 100644 --- a/src/TagFile.cxx +++ b/src/TagFile.cxx @@ -47,11 +47,11 @@ public: handler(_handler), is(nullptr) {} - bool ScanFile(const DecoderPlugin &plugin) noexcept { + bool ScanFile(const DecoderPlugin &plugin) { return plugin.ScanFile(path_fs, handler); } - bool ScanStream(const DecoderPlugin &plugin) noexcept { + bool ScanStream(const DecoderPlugin &plugin) { if (plugin.scan_stream == nullptr) return false; @@ -73,14 +73,14 @@ public: return plugin.ScanStream(*is, handler); } - bool Scan(const DecoderPlugin &plugin) noexcept { + bool Scan(const DecoderPlugin &plugin) { return plugin.SupportsSuffix(suffix) && (ScanFile(plugin) || ScanStream(plugin)); } }; bool -ScanFileTagsNoGeneric(Path path_fs, TagHandler &handler) noexcept +ScanFileTagsNoGeneric(Path path_fs, TagHandler &handler) { assert(!path_fs.IsNull()); @@ -100,7 +100,7 @@ ScanFileTagsNoGeneric(Path path_fs, TagHandler &handler) noexcept bool ScanFileTagsWithGeneric(Path path, TagBuilder &builder, - AudioFormat *audio_format) noexcept + AudioFormat *audio_format) { FullTagHandler h(builder, audio_format); diff --git a/src/TagFile.hxx b/src/TagFile.hxx index 691c3569b..98cbb8bce 100644 --- a/src/TagFile.hxx +++ b/src/TagFile.hxx @@ -30,22 +30,26 @@ class TagBuilder; * but does not fall back to generic scanners (APE and ID3) if no tags * were found (but the file was recognized). * + * Throws on I/O error. + * * @return true if the file was recognized (even if no metadata was * found) */ bool -ScanFileTagsNoGeneric(Path path, TagHandler &handler) noexcept; +ScanFileTagsNoGeneric(Path path, TagHandler &handler); /** * Scan the tags of a song file. Invokes matching decoder plugins, * and falls back to generic scanners (APE and ID3) if no tags were * found (but the file was recognized). * + * Throws on I/O error. + * * @return true if the file was recognized (even if no metadata was * found) */ bool ScanFileTagsWithGeneric(Path path, TagBuilder &builder, - AudioFormat *audio_format=nullptr) noexcept; + AudioFormat *audio_format=nullptr); #endif diff --git a/src/TagStream.cxx b/src/TagStream.cxx index 0cedc72cc..3e760217a 100644 --- a/src/TagStream.cxx +++ b/src/TagStream.cxx @@ -45,7 +45,7 @@ CheckDecoderPlugin(const DecoderPlugin &plugin, } bool -tag_stream_scan(InputStream &is, TagHandler &handler) noexcept +tag_stream_scan(InputStream &is, TagHandler &handler) { assert(is.IsReady()); @@ -73,19 +73,17 @@ tag_stream_scan(InputStream &is, TagHandler &handler) noexcept } bool -tag_stream_scan(const char *uri, TagHandler &handler) noexcept -try { +tag_stream_scan(const char *uri, TagHandler &handler) +{ Mutex mutex; auto is = InputStream::OpenReady(uri, mutex); return tag_stream_scan(*is, handler); -} catch (const std::exception &e) { - return false; } bool tag_stream_scan(InputStream &is, TagBuilder &builder, - AudioFormat *audio_format) noexcept + AudioFormat *audio_format) { assert(is.IsReady()); @@ -102,12 +100,10 @@ tag_stream_scan(InputStream &is, TagBuilder &builder, bool tag_stream_scan(const char *uri, TagBuilder &builder, - AudioFormat *audio_format) noexcept -try { + AudioFormat *audio_format) +{ Mutex mutex; auto is = InputStream::OpenReady(uri, mutex); return tag_stream_scan(*is, builder, audio_format); -} catch (const std::exception &e) { - return false; } diff --git a/src/TagStream.hxx b/src/TagStream.hxx index 33e513f16..f4cd43a5a 100644 --- a/src/TagStream.hxx +++ b/src/TagStream.hxx @@ -29,29 +29,39 @@ class TagBuilder; * Scan the tags of an #InputStream. Invokes matching decoder * plugins, but does not invoke the special "APE" and "ID3" scanners. * + * Throws on I/O error. + * * @return true if the file was recognized (even if no metadata was * found) */ bool -tag_stream_scan(InputStream &is, TagHandler &handler) noexcept; +tag_stream_scan(InputStream &is, TagHandler &handler); +/** + * Throws on I/O error. + */ bool -tag_stream_scan(const char *uri, TagHandler &handler) noexcept; +tag_stream_scan(const char *uri, TagHandler &handler); /** * Scan the tags of an #InputStream. Invokes matching decoder * plugins, and falls back to generic scanners (APE and ID3) if no * tags were found (but the file was recognized). * + * Throws on I/O error. + * * @return true if the file was recognized (even if no metadata was * found) */ bool tag_stream_scan(InputStream &is, TagBuilder &builder, - AudioFormat *audio_format=nullptr) noexcept; + AudioFormat *audio_format=nullptr); +/** + * Throws on I/O error. + */ bool tag_stream_scan(const char *uri, TagBuilder &builder, - AudioFormat *audio_format=nullptr) noexcept; + AudioFormat *audio_format=nullptr); #endif diff --git a/src/decoder/DecoderPlugin.hxx b/src/decoder/DecoderPlugin.hxx index c4b023018..9951992c6 100644 --- a/src/decoder/DecoderPlugin.hxx +++ b/src/decoder/DecoderPlugin.hxx @@ -67,18 +67,22 @@ struct DecoderPlugin { void (*file_decode)(DecoderClient &client, Path path_fs); /** - * Scan metadata of a file. + * Scan metadata of a file. + * + * Throws on I/O error. * - * @return false if the operation has failed + * @return false if the file was not recognized */ - bool (*scan_file)(Path path_fs, TagHandler &handler) noexcept; + bool (*scan_file)(Path path_fs, TagHandler &handler); /** - * Scan metadata of a file. + * Scan metadata of a stream. + * + * Throws on I/O error. * - * @return false if the operation has failed + * @return false if the stream was not recognized */ - bool (*scan_stream)(InputStream &is, TagHandler &handler) noexcept; + bool (*scan_stream)(InputStream &is, TagHandler &handler); /** * @brief Return a "virtual" filename for subtracks in @@ -135,7 +139,7 @@ struct DecoderPlugin { * Read the tag of a file. */ template - bool ScanFile(P path_fs, TagHandler &handler) const noexcept { + bool ScanFile(P path_fs, TagHandler &handler) const { return scan_file != nullptr ? scan_file(path_fs, handler) : false; @@ -144,7 +148,7 @@ struct DecoderPlugin { /** * Read the tag of a stream. */ - bool ScanStream(InputStream &is, TagHandler &handler) const noexcept { + bool ScanStream(InputStream &is, TagHandler &handler) const { return scan_stream != nullptr ? scan_stream(is, handler) : false; diff --git a/src/decoder/plugins/AudiofileDecoderPlugin.cxx b/src/decoder/plugins/AudiofileDecoderPlugin.cxx index 3ecfb4bba..e32b161f9 100644 --- a/src/decoder/plugins/AudiofileDecoderPlugin.cxx +++ b/src/decoder/plugins/AudiofileDecoderPlugin.cxx @@ -241,7 +241,7 @@ audiofile_stream_decode(DecoderClient &client, InputStream &is) } static bool -audiofile_scan_stream(InputStream &is, TagHandler &handler) noexcept +audiofile_scan_stream(InputStream &is, TagHandler &handler) { if (!is.IsSeekable() || !is.KnownSize()) return false; diff --git a/src/decoder/plugins/DsdiffDecoderPlugin.cxx b/src/decoder/plugins/DsdiffDecoderPlugin.cxx index caca7f7ba..c5b72effb 100644 --- a/src/decoder/plugins/DsdiffDecoderPlugin.cxx +++ b/src/decoder/plugins/DsdiffDecoderPlugin.cxx @@ -449,7 +449,7 @@ dsdiff_stream_decode(DecoderClient &client, InputStream &is) } static bool -dsdiff_scan_stream(InputStream &is, TagHandler &handler) noexcept +dsdiff_scan_stream(InputStream &is, TagHandler &handler) { DsdiffMetaData metadata; DsdiffChunkHeader chunk_header; diff --git a/src/decoder/plugins/DsfDecoderPlugin.cxx b/src/decoder/plugins/DsfDecoderPlugin.cxx index d95fda9da..d57644f6f 100644 --- a/src/decoder/plugins/DsfDecoderPlugin.cxx +++ b/src/decoder/plugins/DsfDecoderPlugin.cxx @@ -326,7 +326,7 @@ dsf_stream_decode(DecoderClient &client, InputStream &is) } static bool -dsf_scan_stream(InputStream &is, TagHandler &handler) noexcept +dsf_scan_stream(InputStream &is, TagHandler &handler) { /* check DSF metadata */ DsfMetaData metadata; diff --git a/src/decoder/plugins/FaadDecoderPlugin.cxx b/src/decoder/plugins/FaadDecoderPlugin.cxx index 2f3d4e991..e4f6e9c0c 100644 --- a/src/decoder/plugins/FaadDecoderPlugin.cxx +++ b/src/decoder/plugins/FaadDecoderPlugin.cxx @@ -414,7 +414,7 @@ faad_stream_decode(DecoderClient &client, InputStream &is) } static bool -faad_scan_stream(InputStream &is, TagHandler &handler) noexcept +faad_scan_stream(InputStream &is, TagHandler &handler) { auto result = faad_get_file_time(is); if (!result.first) diff --git a/src/decoder/plugins/FfmpegDecoderPlugin.cxx b/src/decoder/plugins/FfmpegDecoderPlugin.cxx index fa7ae1371..77ad67826 100644 --- a/src/decoder/plugins/FfmpegDecoderPlugin.cxx +++ b/src/decoder/plugins/FfmpegDecoderPlugin.cxx @@ -646,8 +646,7 @@ ffmpeg_decode(DecoderClient &client, InputStream &input) } static bool -FfmpegScanStream(AVFormatContext &format_context, - TagHandler &handler) noexcept +FfmpegScanStream(AVFormatContext &format_context, TagHandler &handler) { const int find_result = avformat_find_stream_info(&format_context, nullptr); @@ -680,7 +679,7 @@ FfmpegScanStream(AVFormatContext &format_context, } static bool -ffmpeg_scan_stream(InputStream &is, TagHandler &handler) noexcept +ffmpeg_scan_stream(InputStream &is, TagHandler &handler) { AvioStream stream(nullptr, is); if (!stream.Open()) diff --git a/src/decoder/plugins/FlacDecoderPlugin.cxx b/src/decoder/plugins/FlacDecoderPlugin.cxx index 176d9bcf3..b7cf03f48 100644 --- a/src/decoder/plugins/FlacDecoderPlugin.cxx +++ b/src/decoder/plugins/FlacDecoderPlugin.cxx @@ -69,7 +69,7 @@ flac_write_cb(const FLAC__StreamDecoder *dec, const FLAC__Frame *frame, } static bool -flac_scan_file(Path path_fs, TagHandler &handler) noexcept +flac_scan_file(Path path_fs, TagHandler &handler) { FlacMetadataChain chain; if (!chain.Read(NarrowPath(path_fs))) { @@ -84,7 +84,7 @@ flac_scan_file(Path path_fs, TagHandler &handler) noexcept } static bool -flac_scan_stream(InputStream &is, TagHandler &handler) noexcept +flac_scan_stream(InputStream &is, TagHandler &handler) { FlacMetadataChain chain; if (!chain.Read(is)) { @@ -313,7 +313,7 @@ oggflac_init(gcc_unused const ConfigBlock &block) } static bool -oggflac_scan_file(Path path_fs, TagHandler &handler) noexcept +oggflac_scan_file(Path path_fs, TagHandler &handler) { FlacMetadataChain chain; if (!chain.ReadOgg(NarrowPath(path_fs))) { @@ -328,7 +328,7 @@ oggflac_scan_file(Path path_fs, TagHandler &handler) noexcept } static bool -oggflac_scan_stream(InputStream &is, TagHandler &handler) noexcept +oggflac_scan_stream(InputStream &is, TagHandler &handler) { FlacMetadataChain chain; if (!chain.ReadOgg(is)) { diff --git a/src/decoder/plugins/MadDecoderPlugin.cxx b/src/decoder/plugins/MadDecoderPlugin.cxx index b21f7cba9..b5a688bbf 100644 --- a/src/decoder/plugins/MadDecoderPlugin.cxx +++ b/src/decoder/plugins/MadDecoderPlugin.cxx @@ -1051,7 +1051,7 @@ MadDecoder::RunScan(TagHandler &handler) noexcept } static bool -mad_decoder_scan_stream(InputStream &is, TagHandler &handler) noexcept +mad_decoder_scan_stream(InputStream &is, TagHandler &handler) { MadDecoder data(nullptr, is); return data.RunScan(handler); diff --git a/src/decoder/plugins/MpcdecDecoderPlugin.cxx b/src/decoder/plugins/MpcdecDecoderPlugin.cxx index 68ecc115d..1981fb9fc 100644 --- a/src/decoder/plugins/MpcdecDecoderPlugin.cxx +++ b/src/decoder/plugins/MpcdecDecoderPlugin.cxx @@ -275,7 +275,7 @@ mpcdec_get_file_duration(InputStream &is) } static bool -mpcdec_scan_stream(InputStream &is, TagHandler &handler) noexcept +mpcdec_scan_stream(InputStream &is, TagHandler &handler) { const auto duration = mpcdec_get_file_duration(is); if (duration.IsNegative()) diff --git a/src/decoder/plugins/OpusDecoderPlugin.cxx b/src/decoder/plugins/OpusDecoderPlugin.cxx index fa448a2ff..5284ac9a1 100644 --- a/src/decoder/plugins/OpusDecoderPlugin.cxx +++ b/src/decoder/plugins/OpusDecoderPlugin.cxx @@ -424,7 +424,7 @@ VisitOpusDuration(InputStream &is, OggSyncState &sync, OggStreamState &stream, } static bool -mpd_opus_scan_stream(InputStream &is, TagHandler &handler) noexcept +mpd_opus_scan_stream(InputStream &is, TagHandler &handler) { InputStreamReader reader(is); OggSyncState oy(reader); diff --git a/src/decoder/plugins/SndfileDecoderPlugin.cxx b/src/decoder/plugins/SndfileDecoderPlugin.cxx index de0f45112..efcf49914 100644 --- a/src/decoder/plugins/SndfileDecoderPlugin.cxx +++ b/src/decoder/plugins/SndfileDecoderPlugin.cxx @@ -267,7 +267,7 @@ static constexpr struct { }; static bool -sndfile_scan_stream(InputStream &is, TagHandler &handler) noexcept +sndfile_scan_stream(InputStream &is, TagHandler &handler) { SF_INFO info; diff --git a/src/decoder/plugins/VorbisDecoderPlugin.cxx b/src/decoder/plugins/VorbisDecoderPlugin.cxx index be90fc89b..9340394b7 100644 --- a/src/decoder/plugins/VorbisDecoderPlugin.cxx +++ b/src/decoder/plugins/VorbisDecoderPlugin.cxx @@ -370,7 +370,7 @@ VisitVorbisDuration(InputStream &is, } static bool -vorbis_scan_stream(InputStream &is, TagHandler &handler) noexcept +vorbis_scan_stream(InputStream &is, TagHandler &handler) { /* initialize libogg */ diff --git a/src/decoder/plugins/WavpackDecoderPlugin.cxx b/src/decoder/plugins/WavpackDecoderPlugin.cxx index 1e65c0fcd..087b08fb5 100644 --- a/src/decoder/plugins/WavpackDecoderPlugin.cxx +++ b/src/decoder/plugins/WavpackDecoderPlugin.cxx @@ -614,7 +614,7 @@ wavpack_scan_file(Path path_fs, TagHandler &handler) noexcept } static bool -wavpack_scan_stream(InputStream &is, TagHandler &handler) noexcept +wavpack_scan_stream(InputStream &is, TagHandler &handler) { WavpackInput isp(nullptr, is);