From bfdf13dca365fb6ac9a5912d6a1a9efb08b2e361 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 13:36:22 +0200 Subject: [PATCH 01/12] 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); From 6517b2d2acedb7226802b423e2131f83a693d3be Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 16:09:17 +0200 Subject: [PATCH 02/12] neighbor/upnp: remove D-Bus filter and match in Close() Fixes use-after-free crash bug during MPD shutdown. --- NEWS | 2 ++ src/neighbor/plugins/UdisksNeighborPlugin.cxx | 17 ++++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/NEWS b/NEWS index d4e05e837..a515aeaad 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,8 @@ ver 0.21.25 (not yet released) * output - osx: improve sample rate selection - osx: fix noise while stopping +* neighbor + - upnp: fix crash during shutdown * Windows/Android: - fix Boost detection after breaking change in Meson 0.54 diff --git a/src/neighbor/plugins/UdisksNeighborPlugin.cxx b/src/neighbor/plugins/UdisksNeighborPlugin.cxx index cb5d422f2..359d16ef3 100644 --- a/src/neighbor/plugins/UdisksNeighborPlugin.cxx +++ b/src/neighbor/plugins/UdisksNeighborPlugin.cxx @@ -47,6 +47,11 @@ ToNeighborInfo(const UDisks2::Object &o) noexcept return {o.GetUri(), o.path}; } +static constexpr char udisks_neighbor_match[] = + "type='signal',sender='" UDISKS2_INTERFACE "'," + "interface='" DBUS_OM_INTERFACE "'," + "path='" UDISKS2_PATH "'"; + class UdisksNeighborExplorer final : public NeighborExplorer { @@ -110,11 +115,7 @@ UdisksNeighborExplorer::DoOpen() try { Error error; - dbus_bus_add_match(connection, - "type='signal',sender='" UDISKS2_INTERFACE "'," - "interface='" DBUS_OM_INTERFACE "'," - "path='" UDISKS2_PATH "'", - error); + dbus_bus_add_match(connection, udisks_neighbor_match, error); error.CheckThrow("DBus AddMatch error"); dbus_connection_add_filter(connection, @@ -147,8 +148,10 @@ UdisksNeighborExplorer::DoClose() noexcept list_request.Cancel(); } - // TODO: remove_match - // TODO: remove_filter + auto &connection = GetConnection(); + + dbus_connection_remove_filter(connection, HandleMessage, this); + dbus_bus_remove_match(connection, udisks_neighbor_match, nullptr); dbus_glue.Destruct(); } From 39d6816a6df4929f90eadcc976a7b9a42d84d027 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 16:15:10 +0200 Subject: [PATCH 03/12] neighbor/upnp: roll back changes if DoOpen() fails --- src/neighbor/plugins/UdisksNeighborPlugin.cxx | 35 +++++++++++++------ 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/neighbor/plugins/UdisksNeighborPlugin.cxx b/src/neighbor/plugins/UdisksNeighborPlugin.cxx index 359d16ef3..f69901f6a 100644 --- a/src/neighbor/plugins/UdisksNeighborPlugin.cxx +++ b/src/neighbor/plugins/UdisksNeighborPlugin.cxx @@ -113,22 +113,37 @@ UdisksNeighborExplorer::DoOpen() auto &connection = GetConnection(); + /* this ugly try/catch cascade is only here because this + method has no RAII for this method - TODO: improve this */ try { Error error; dbus_bus_add_match(connection, udisks_neighbor_match, error); error.CheckThrow("DBus AddMatch error"); - dbus_connection_add_filter(connection, - HandleMessage, this, - nullptr); + try { + dbus_connection_add_filter(connection, + HandleMessage, this, + nullptr); - auto msg = Message::NewMethodCall(UDISKS2_INTERFACE, - UDISKS2_PATH, - DBUS_OM_INTERFACE, - "GetManagedObjects"); - list_request.Send(connection, *msg.Get(), - std::bind(&UdisksNeighborExplorer::OnListNotify, - this, std::placeholders::_1)); + try { + auto msg = Message::NewMethodCall(UDISKS2_INTERFACE, + UDISKS2_PATH, + DBUS_OM_INTERFACE, + "GetManagedObjects"); + list_request.Send(connection, *msg.Get(), + std::bind(&UdisksNeighborExplorer::OnListNotify, + this, std::placeholders::_1)); + } catch (...) { + dbus_connection_remove_filter(connection, + HandleMessage, + this); + throw; + } + } catch (...) { + dbus_bus_remove_match(connection, + udisks_neighbor_match, nullptr); + throw; + } } catch (...) { dbus_glue.Destruct(); throw; From 5b291ff768ded0c4d1e23ec0ba7be9b721b2a16a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 17:18:48 +0200 Subject: [PATCH 04/12] db/update/Walk: pass concatenated .mpdignore URI to storage.MapUTF8() Fixes the "Unrecognized URI" error with the udisks storage plugin, which is caused by the kludge in UdisksStorage::MapUTF8(). --- NEWS | 2 ++ src/db/update/Walk.cxx | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index a515aeaad..efc4f0aa6 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ ver 0.21.25 (not yet released) * protocol: - fix crash when using "rangeid" while playing +* storage + - udisks: fix reading ".mpdignore" * input - file: detect premature end of file - smbclient: don't send credentials to MPD clients diff --git a/src/db/update/Walk.cxx b/src/db/update/Walk.cxx index eb853fcb8..ccc8bb6de 100644 --- a/src/db/update/Walk.cxx +++ b/src/db/update/Walk.cxx @@ -341,8 +341,8 @@ UpdateWalk::UpdateDirectory(Directory &directory, try { Mutex mutex; - auto is = InputStream::OpenReady(PathTraitsUTF8::Build(storage.MapUTF8(directory.GetPath()).c_str(), - ".mpdignore").c_str(), + auto is = InputStream::OpenReady(storage.MapUTF8(PathTraitsUTF8::Build(directory.GetPath(), + ".mpdignore").c_str()).c_str(), mutex); child_exclude_list.Load(std::move(is)); } catch (...) { From 33ee35ab928fab752aa7f533e6a9a2b46de6528a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 17:45:59 +0200 Subject: [PATCH 05/12] command/storage: check if mount point is busy When mounting something over a directory that is already a mount point, CompositeStorage::Mount() silently overwrites the previously mounted storage, disposing it. After that, SimpleDatabase::Mount() will fail and handle_mount() will roll back the CompositeStorage::Mount() command, effectively unmounting what was there before (and also leaking memory). Closes https://github.com/MusicPlayerDaemon/MPD/issues/918 --- NEWS | 1 + src/command/StorageCommands.cxx | 5 +++++ src/storage/CompositeStorage.cxx | 1 + src/storage/CompositeStorage.hxx | 9 +++++++++ src/storage/StorageState.cxx | 9 +++++++-- 5 files changed, 23 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index efc4f0aa6..b0447c9b0 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,7 @@ ver 0.21.25 (not yet released) * protocol: - fix crash when using "rangeid" while playing * storage + - fix disappearing mounts after mounting twice - udisks: fix reading ".mpdignore" * input - file: detect premature end of file diff --git a/src/command/StorageCommands.cxx b/src/command/StorageCommands.cxx index 7926b66c3..288f341d0 100644 --- a/src/command/StorageCommands.cxx +++ b/src/command/StorageCommands.cxx @@ -198,6 +198,11 @@ handle_mount(Client &client, Request args, Response &r) return CommandResult::ERROR; } + if (composite.IsMountPoint(local_uri)) { + r.Error(ACK_ERROR_ARG, "Mount point busy"); + return CommandResult::ERROR; + } + auto &event_loop = instance.io_thread.GetEventLoop(); auto storage = CreateStorageURI(event_loop, remote_uri); if (storage == nullptr) { diff --git a/src/storage/CompositeStorage.cxx b/src/storage/CompositeStorage.cxx index 40113af18..ff47e6563 100644 --- a/src/storage/CompositeStorage.cxx +++ b/src/storage/CompositeStorage.cxx @@ -206,6 +206,7 @@ CompositeStorage::Mount(const char *uri, std::unique_ptr storage) const std::lock_guard protect(mutex); Directory &directory = root.Make(uri); + assert(!directory.storage); directory.storage = std::move(storage); } diff --git a/src/storage/CompositeStorage.hxx b/src/storage/CompositeStorage.hxx index cb5d49d6a..d6c424d54 100644 --- a/src/storage/CompositeStorage.hxx +++ b/src/storage/CompositeStorage.hxx @@ -100,6 +100,15 @@ public: gcc_pure gcc_nonnull_all Storage *GetMount(const char *uri) noexcept; + /** + * Is the given URI a mount point, i.e. is something already + * mounted on this path? + */ + gcc_pure gcc_nonnull_all + bool IsMountPoint(const char *uri) noexcept { + return GetMount(uri) != nullptr; + } + /** * Call the given function for each mounted storage, including * the root storage. Passes mount point URI and the a const diff --git a/src/storage/StorageState.cxx b/src/storage/StorageState.cxx index d6a63601e..34e669787 100644 --- a/src/storage/StorageState.cxx +++ b/src/storage/StorageState.cxx @@ -106,6 +106,12 @@ storage_state_restore(const char *line, TextFile &file, Instance &instance) FormatDebug(storage_domain, "Restoring mount %s => %s", uri.c_str(), url.c_str()); + auto &composite_storage = *(CompositeStorage *)instance.storage; + if (composite_storage.IsMountPoint(uri.c_str())) { + LogError(storage_domain, "Mount point busy"); + return true; + } + auto &event_loop = instance.io_thread.GetEventLoop(); auto storage = CreateStorageURI(event_loop, url.c_str()); if (storage == nullptr) { @@ -124,8 +130,7 @@ storage_state_restore(const char *line, TextFile &file, Instance &instance) } } - ((CompositeStorage*)instance.storage)->Mount(uri.c_str(), - std::move(storage)); + composite_storage.Mount(uri.c_str(), std::move(storage)); return true; } From d7744d2b8e8219e6f9a9fad7eff63c8e5f3a000d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 18:00:57 +0200 Subject: [PATCH 06/12] command/storage: check if storage is already mounted Mounting one storage URI twice on different mount points can lead to conflicts with the database cache file, and it doesn't make a lot of sense. But most importantly, our udisks storage plugin will unmount the disk from the kernel VFS, and if two exist, they will compete with each others. We could (and should) fix this in the udisks storage plugin, but for now, this workaround is good enough (and useful). --- src/command/StorageCommands.cxx | 5 +++++ src/storage/CompositeStorage.hxx | 25 +++++++++++++++++++++++++ src/storage/StorageState.cxx | 5 +++++ 3 files changed, 35 insertions(+) diff --git a/src/command/StorageCommands.cxx b/src/command/StorageCommands.cxx index 288f341d0..2dea85e8d 100644 --- a/src/command/StorageCommands.cxx +++ b/src/command/StorageCommands.cxx @@ -203,6 +203,11 @@ handle_mount(Client &client, Request args, Response &r) return CommandResult::ERROR; } + if (composite.IsMounted(remote_uri)) { + r.Error(ACK_ERROR_ARG, "This storage is already mounted"); + return CommandResult::ERROR; + } + auto &event_loop = instance.io_thread.GetEventLoop(); auto storage = CreateStorageURI(event_loop, remote_uri); if (storage == nullptr) { diff --git a/src/storage/CompositeStorage.hxx b/src/storage/CompositeStorage.hxx index d6c424d54..c06240403 100644 --- a/src/storage/CompositeStorage.hxx +++ b/src/storage/CompositeStorage.hxx @@ -121,6 +121,15 @@ public: VisitMounts(uri, root, t); } + /** + * Is a storage with the given URI already mounted? + */ + gcc_pure gcc_nonnull_all + bool IsMounted(const char *storage_uri) const noexcept { + const std::lock_guard protect(mutex); + return IsMounted(root, storage_uri); + } + void Mount(const char *uri, std::unique_ptr storage); bool Unmount(const char *uri); @@ -155,6 +164,22 @@ private: } } + gcc_pure gcc_nonnull_all + static bool IsMounted(const Directory &directory, + const char *storage_uri) noexcept { + if (directory.storage) { + const auto uri = directory.storage->MapUTF8(""); + if (uri == storage_uri) + return true; + } + + for (const auto &i : directory.children) + if (IsMounted(i.second, storage_uri)) + return true; + + return false; + } + /** * Follow the given URI path, and find the outermost directory * which is a #Storage mount point. If there are no mounts, diff --git a/src/storage/StorageState.cxx b/src/storage/StorageState.cxx index 34e669787..404dfeeb3 100644 --- a/src/storage/StorageState.cxx +++ b/src/storage/StorageState.cxx @@ -112,6 +112,11 @@ storage_state_restore(const char *line, TextFile &file, Instance &instance) return true; } + if (composite_storage.IsMounted(url.c_str())) { + LogError(storage_domain, "This storage is already mounted"); + return true; + } + auto &event_loop = instance.io_thread.GetEventLoop(); auto storage = CreateStorageURI(event_loop, url.c_str()); if (storage == nullptr) { From fe48e5596f9ba0ad93b9995d512b099d324087e3 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 20:04:35 +0200 Subject: [PATCH 07/12] command/storage: automatically scan new mounts Closes https://github.com/MusicPlayerDaemon/MPD/issues/841 --- NEWS | 2 ++ src/command/StorageCommands.cxx | 10 +++++++++- src/db/plugins/simple/SimpleDatabasePlugin.cxx | 6 ++++-- src/db/plugins/simple/SimpleDatabasePlugin.hxx | 4 +++- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index b0447c9b0..054059bfb 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ ver 0.21.25 (not yet released) * protocol: - fix crash when using "rangeid" while playing +* database + - simple: automatically scan new mounts * storage - fix disappearing mounts after mounting twice - udisks: fix reading ".mpdignore" diff --git a/src/command/StorageCommands.cxx b/src/command/StorageCommands.cxx index 2dea85e8d..4d5a58c96 100644 --- a/src/command/StorageCommands.cxx +++ b/src/command/StorageCommands.cxx @@ -220,8 +220,10 @@ handle_mount(Client &client, Request args, Response &r) #ifdef ENABLE_DATABASE if (auto *db = dynamic_cast(instance.GetDatabase())) { + bool need_update; + try { - db->Mount(local_uri, remote_uri); + need_update = !db->Mount(local_uri, remote_uri); } catch (...) { composite.Unmount(local_uri); throw; @@ -230,6 +232,12 @@ handle_mount(Client &client, Request args, Response &r) // TODO: call Instance::OnDatabaseModified()? // TODO: trigger database update? instance.EmitIdle(IDLE_DATABASE); + + if (need_update) { + UpdateService *update = client.GetInstance().update; + if (update != nullptr) + update->Enqueue(local_uri, false); + } } #endif diff --git a/src/db/plugins/simple/SimpleDatabasePlugin.cxx b/src/db/plugins/simple/SimpleDatabasePlugin.cxx index 5350db6d8..4cae15c02 100644 --- a/src/db/plugins/simple/SimpleDatabasePlugin.cxx +++ b/src/db/plugins/simple/SimpleDatabasePlugin.cxx @@ -428,7 +428,7 @@ IsUnsafeChar(char ch) return !IsSafeChar(ch); } -void +bool SimpleDatabase::Mount(const char *local_uri, const char *storage_uri) { if (cache_path.IsNull()) @@ -447,9 +447,11 @@ SimpleDatabase::Mount(const char *local_uri, const char *storage_uri) compress); db->Open(); - // TODO: update the new database instance? + bool exists = db->FileExists(); Mount(local_uri, std::move(db)); + + return exists; } inline DatabasePtr diff --git a/src/db/plugins/simple/SimpleDatabasePlugin.hxx b/src/db/plugins/simple/SimpleDatabasePlugin.hxx index bdc90f6c1..5e525a999 100644 --- a/src/db/plugins/simple/SimpleDatabasePlugin.hxx +++ b/src/db/plugins/simple/SimpleDatabasePlugin.hxx @@ -103,9 +103,11 @@ public: /** * Throws #std::runtime_error on error. + * + * @return false if the mounted database needs to be updated */ gcc_nonnull_all - void Mount(const char *local_uri, const char *storage_uri); + bool Mount(const char *local_uri, const char *storage_uri); gcc_nonnull_all bool Unmount(const char *uri) noexcept; From 996714d6ffc78d8fe999b3b7da7e72830a2eaa50 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 20:29:08 +0200 Subject: [PATCH 08/12] doc/plugins.rst: more markup --- doc/plugins.rst | 51 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/doc/plugins.rst b/doc/plugins.rst index 7268663a4..9a56961b5 100644 --- a/doc/plugins.rst +++ b/doc/plugins.rst @@ -60,25 +60,35 @@ The default plugin which gives :program:`MPD` access to local files. It is used curl ---- -A WebDAV client using libcurl. It is used when :code:`music_directory` contains a http:// or https:// URI, for example :samp:`https://the.server/dav/`. +A WebDAV client using libcurl. It is used when :code:`music_directory` +contains a ``http://`` or ``https://`` URI, for example +:samp:`https://the.server/dav/`. smbclient --------- -Load music files from a SMB/CIFS server. It is used when :code:`music_directory` contains a smb:// URI, for example :samp:`smb://myfileserver/Music`. +Load music files from a SMB/CIFS server. It is used when +:code:`music_directory` contains a ``smb://`` URI, for example +:samp:`smb://myfileserver/Music`. nfs --- Load music files from a NFS server. It is used when :code:`music_directory` contains a nfs:// URI according to RFC2224, for example :samp:`nfs://servername/path`. -This plugin uses libnfs, which supports only NFS version 3. Since :program:`MPD` is not allowed to bind to "privileged ports", the NFS server needs to enable the "insecure" setting; example :file:`/etc/exports`: +This plugin uses libnfs, which supports only NFS version 3. Since +:program:`MPD` is not allowed to bind to "privileged ports", the NFS +server needs to enable the ``insecure`` setting; example +:file:`/etc/exports`: .. code-block:: none /srv/mp3 192.168.1.55(ro,insecure) -Don't fear: "insecure" does not mean that your NFS server is insecure. A few decades ago, people thought the concept of "privileged ports" would make network services "secure", which was a fallacy. The absence of this obsolete "security" measure means little. +Don't fear: ``insecure`` does not mean that your NFS server is +insecure. A few decades ago, people thought the concept of "privileged +ports" would make network services "secure", which was a fallacy. The +absence of this obsolete "security" measure means little. udisks ------ @@ -162,7 +172,10 @@ curl Opens remote files or streams over HTTP using libcurl. -Note that unless overridden by the below settings (e.g. by setting them to a blank value), general curl configuration from environment variables such as http_proxy or specified in :file:`~/.curlrc` will be in effect. +Note that unless overridden by the below settings (e.g. by setting +them to a blank value), general curl configuration from environment +variables such as ``http_proxy`` or specified in :file:`~/.curlrc` +will be in effect. .. list-table:: :widths: 20 80 @@ -182,7 +195,9 @@ Note that unless overridden by the below settings (e.g. by setting them to a bla ffmpeg ------ -Access to various network protocols implemented by the FFmpeg library: gopher://, rtp://, rtsp://, rtmp://, rtmpt://, rtmps:// +Access to various network protocols implemented by the FFmpeg library: +``gopher://``, ``rtp://``, ``rtsp://``, ``rtmp://``, ``rtmpt://``, +``rtmps://`` file ---- @@ -197,18 +212,29 @@ Plays streams with the MMS protocol using `libmms nfs --- -Allows :program:`MPD` to access files on NFSv3 servers without actually mounting them (i.e. in userspace, without help from the kernel's VFS layer). All URIs with the nfs:// scheme are used according to RFC2224. Example: +Allows :program:`MPD` to access files on NFSv3 servers without +actually mounting them (i.e. in userspace, without help from the +kernel's VFS layer). All URIs with the ``nfs://`` scheme are used +according to RFC2224. Example: .. code-block:: none mpc add nfs://servername/path/filename.ogg -Note that this usually requires enabling the "insecure" flag in the server's /etc/exports file, because :program:`MPD` cannot bind to so-called "privileged" ports. Don't fear: this will not make your file server insecure; the flag was named in a time long ago when privileged ports were thought to be meaningful for security. By today's standards, NFSv3 is not secure at all, and if you believe it is, you're already doomed. +Note that this usually requires enabling the ``insecure`` flag in the +server's /etc/exports file, because :program:`MPD` cannot bind to +so-called "privileged" ports. Don't fear: this will not make your file +server insecure; the flag was named in a time long ago when privileged +ports were thought to be meaningful for security. By today's +standards, NFSv3 is not secure at all, and if you believe it is, +you're already doomed. smbclient --------- -Allows :program:`MPD` to access files on SMB/CIFS servers (e.g. Samba or Microsoft Windows). All URIs with the smb:// scheme are used. Example: +Allows :program:`MPD` to access files on SMB/CIFS servers (e.g. Samba +or Microsoft Windows). All URIs with the ``smb://`` scheme are +used. Example: .. code-block:: none @@ -217,7 +243,8 @@ Allows :program:`MPD` to access files on SMB/CIFS servers (e.g. Samba or Microso qobuz ----- -Play songs from the commercial streaming service Qobuz. It plays URLs in the form qobuz://track/ID, e.g.: +Play songs from the commercial streaming service Qobuz. It plays URLs +in the form ``qobuz://track/ID``, e.g.: .. code-block:: none @@ -243,7 +270,9 @@ Play songs from the commercial streaming service Qobuz. It plays URLs in the for tidal ----- -Play songs from the commercial streaming service `Tidal `_. It plays URLs in the form tidal://track/ID, e.g.: +Play songs from the commercial streaming service `Tidal +`_. It plays URLs in the form ``tidal://track/ID``, +e.g.: .. warning:: From eaa66c7ee386013f4f99c970de55a47defacf15f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 20:32:31 +0200 Subject: [PATCH 09/12] doc/plugins.rst: add smb:// with password example Closes https://github.com/MusicPlayerDaemon/MPD/issues/864 --- doc/plugins.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/plugins.rst b/doc/plugins.rst index 9a56961b5..0094f2f73 100644 --- a/doc/plugins.rst +++ b/doc/plugins.rst @@ -239,6 +239,7 @@ used. Example: .. code-block:: none mpc add smb://servername/sharename/filename.ogg + mpc add smb://username:password@servername/sharename/filename.ogg qobuz ----- From 402663de749a2088bffa85eadb1a5abb0e237b2a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 20:34:03 +0200 Subject: [PATCH 10/12] doc/plugins.rst: more markup --- doc/plugins.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/plugins.rst b/doc/plugins.rst index 0094f2f73..cacad9fb6 100644 --- a/doc/plugins.rst +++ b/doc/plugins.rst @@ -74,7 +74,9 @@ Load music files from a SMB/CIFS server. It is used when nfs --- -Load music files from a NFS server. It is used when :code:`music_directory` contains a nfs:// URI according to RFC2224, for example :samp:`nfs://servername/path`. +Load music files from a NFS server. It is used when +:code:`music_directory` contains a ``nfs://`` URI according to +RFC2224, for example :samp:`nfs://servername/path`. This plugin uses libnfs, which supports only NFS version 3. Since :program:`MPD` is not allowed to bind to "privileged ports", the NFS From 0b59f4eaee215480da3acf39f7b4df06cf57f931 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 20:34:14 +0200 Subject: [PATCH 11/12] doc/plugins.rst: merge redundant nfs:// documentation --- doc/plugins.rst | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/doc/plugins.rst b/doc/plugins.rst index cacad9fb6..5690cf5c0 100644 --- a/doc/plugins.rst +++ b/doc/plugins.rst @@ -78,19 +78,7 @@ Load music files from a NFS server. It is used when :code:`music_directory` contains a ``nfs://`` URI according to RFC2224, for example :samp:`nfs://servername/path`. -This plugin uses libnfs, which supports only NFS version 3. Since -:program:`MPD` is not allowed to bind to "privileged ports", the NFS -server needs to enable the ``insecure`` setting; example -:file:`/etc/exports`: - -.. code-block:: none - - /srv/mp3 192.168.1.55(ro,insecure) - -Don't fear: ``insecure`` does not mean that your NFS server is -insecure. A few decades ago, people thought the concept of "privileged -ports" would make network services "secure", which was a fallacy. The -absence of this obsolete "security" measure means little. +See :ref:`input_nfs` for more information. udisks ------ @@ -211,25 +199,33 @@ mms Plays streams with the MMS protocol using `libmms `_. +.. _input_nfs: + nfs --- -Allows :program:`MPD` to access files on NFSv3 servers without -actually mounting them (i.e. in userspace, without help from the -kernel's VFS layer). All URIs with the ``nfs://`` scheme are used -according to RFC2224. Example: +Allows :program:`MPD` to access files on NFS servers without actually +mounting them (i.e. with :program:`libnfs` in userspace, without help +from the kernel's VFS layer). All URIs with the ``nfs://`` scheme are +used according to RFC2224. Example: .. code-block:: none mpc add nfs://servername/path/filename.ogg -Note that this usually requires enabling the ``insecure`` flag in the -server's /etc/exports file, because :program:`MPD` cannot bind to -so-called "privileged" ports. Don't fear: this will not make your file -server insecure; the flag was named in a time long ago when privileged -ports were thought to be meaningful for security. By today's -standards, NFSv3 is not secure at all, and if you believe it is, -you're already doomed. +This plugin uses :program:`libnfs`, which supports only NFS version 3. +Since :program:`MPD` is not allowed to bind to so-called "privileged +ports", the NFS server needs to enable the ``insecure`` setting; +example :file:`/etc/exports`: + +.. code-block:: none + + /srv/mp3 192.168.1.55(ro,insecure) + +Don't fear: this will not make your file server insecure; the flag was +named a time long ago when privileged ports were thought to be +meaningful for security. By today's standards, NFSv3 is not secure at +all, and if you believe it is, you're already doomed. smbclient --------- From 749ad7cd83f4061fc8714202bcc51b3ec065139d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 6 Jul 2020 20:40:07 +0200 Subject: [PATCH 12/12] PluginUnavailable: inherit the base class constructor --- src/PluginUnavailable.hxx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/PluginUnavailable.hxx b/src/PluginUnavailable.hxx index a562b3890..379bd87bf 100644 --- a/src/PluginUnavailable.hxx +++ b/src/PluginUnavailable.hxx @@ -1,5 +1,5 @@ /* - * Copyright 2003-2018 The Music Player Daemon Project + * Copyright 2003-2020 The Music Player Daemon Project * http://www.musicpd.org * * This program is free software; you can redistribute it and/or modify @@ -29,8 +29,7 @@ */ class PluginUnavailable final : public std::runtime_error { public: - explicit PluginUnavailable(const char *msg) - :std::runtime_error(msg) {} + using std::runtime_error::runtime_error; }; #endif