From 20ae84bff9db62f29d2b23a4b06f0c65641d0f84 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 10 Feb 2017 15:05:49 +0100 Subject: [PATCH 1/6] {input,mixer}/alsa: cancel the DeferredMonitor in the destructor Yet another potential crash bug fix. --- src/input/plugins/AlsaInputPlugin.cxx | 1 + src/mixer/plugins/AlsaMixerPlugin.cxx | 1 + 2 files changed, 2 insertions(+) diff --git a/src/input/plugins/AlsaInputPlugin.cxx b/src/input/plugins/AlsaInputPlugin.cxx index 40967dd40..4cbf1644c 100644 --- a/src/input/plugins/AlsaInputPlugin.cxx +++ b/src/input/plugins/AlsaInputPlugin.cxx @@ -100,6 +100,7 @@ public: ~AlsaInputStream() { BlockingCall(MultiSocketMonitor::GetEventLoop(), [this](){ MultiSocketMonitor::Reset(); + DeferredMonitor::Cancel(); }); snd_pcm_close(capture_handle); diff --git a/src/mixer/plugins/AlsaMixerPlugin.cxx b/src/mixer/plugins/AlsaMixerPlugin.cxx index 852326926..85ccdf8dd 100644 --- a/src/mixer/plugins/AlsaMixerPlugin.cxx +++ b/src/mixer/plugins/AlsaMixerPlugin.cxx @@ -57,6 +57,7 @@ public: ~AlsaMixerMonitor() { BlockingCall(MultiSocketMonitor::GetEventLoop(), [this](){ MultiSocketMonitor::Reset(); + DeferredMonitor::Cancel(); }); } From d84eaeafc5a08592abd56417bc24c21cb0ddc410 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 18 Feb 2017 19:01:04 +0100 Subject: [PATCH 2/6] doc/include/tags.xml: clarify that track/disc are decimal --- doc/include/tags.xml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/include/tags.xml b/doc/include/tags.xml index 554b43a82..b3554efb9 100644 --- a/doc/include/tags.xml +++ b/doc/include/tags.xml @@ -55,7 +55,8 @@ - track: the track number within the album. + track: the decimal track number within the + album. @@ -103,7 +104,8 @@ - disc: the disc number in a multi-disc album. + disc: the decimal disc number in a multi-disc + album. From 1bd00b8a9ab59982ec80a598dba3aa2eb3ef03a2 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sun, 19 Feb 2017 19:08:06 +0100 Subject: [PATCH 3/6] output/httpd/IcyMetaDataServer: remove the int cast Why did this cast exist?? --- src/output/plugins/httpd/IcyMetaDataServer.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/output/plugins/httpd/IcyMetaDataServer.cxx b/src/output/plugins/httpd/IcyMetaDataServer.cxx index e2fbddb85..265305cf6 100644 --- a/src/output/plugins/httpd/IcyMetaDataServer.cxx +++ b/src/output/plugins/httpd/IcyMetaDataServer.cxx @@ -68,7 +68,7 @@ icy_server_metadata_string(const char *stream_title, const char* stream_url) meta_length--; // subtract placeholder - meta_length = ((int)meta_length / 16) + 1; + meta_length = meta_length / 16 + 1; icy_metadata[0] = meta_length; From a73195b7cc7333a1761902a56952f5f3b9832810 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sun, 19 Feb 2017 19:06:48 +0100 Subject: [PATCH 4/6] output/httpd/IcyMetaDataServer: pad the string with 15 spaces Fixes a buffer overflow due to the bad formula rounding the buffer size up. At the same time, remove the "+1" from the meta_length calculation, which takes the padding into account and at the same time implements proper rounding. --- src/output/plugins/httpd/IcyMetaDataServer.cxx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/output/plugins/httpd/IcyMetaDataServer.cxx b/src/output/plugins/httpd/IcyMetaDataServer.cxx index 265305cf6..fbcd07054 100644 --- a/src/output/plugins/httpd/IcyMetaDataServer.cxx +++ b/src/output/plugins/httpd/IcyMetaDataServer.cxx @@ -60,7 +60,11 @@ icy_server_metadata_string(const char *stream_title, const char* stream_url) { // The leading n is a placeholder for the length information auto icy_metadata = FormatString("nStreamTitle='%s';" - "StreamUrl='%s';", + "StreamUrl='%s';" + /* pad 15 spaces just in case + the length needs to be + rounded up */ + " ", stream_title, stream_url); @@ -68,7 +72,7 @@ icy_server_metadata_string(const char *stream_title, const char* stream_url) meta_length--; // subtract placeholder - meta_length = meta_length / 16 + 1; + meta_length = meta_length / 16; icy_metadata[0] = meta_length; From 4bb83781e8b131737cf1de9d861757558b3839ea Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sun, 19 Feb 2017 19:11:04 +0100 Subject: [PATCH 5/6] output/httpd/IcyMetaDataServer: cast length to unsigned Fixes another buffer overflow: if the stream has a very long title or URL, resulting in a metadata string of more than 2 kB, icy_string[0] is a negative value, which gets casted to size_t - ouch! https://bugs.musicpd.org/view.php?id=4652 --- NEWS | 2 ++ src/output/plugins/httpd/IcyMetaDataServer.cxx | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 1fcae0d6d..967bd68f1 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ ver 0.20.5 (not yet released) - id3: fix memory leak on corrupt ID3 tags * decoder - sidplay: don't require libsidutils when building with libsidplayfp +* output + - httpd: fix two buffer overflows in IcyMetaData length calculation * mixer - alsa: fix crash bug diff --git a/src/output/plugins/httpd/IcyMetaDataServer.cxx b/src/output/plugins/httpd/IcyMetaDataServer.cxx index fbcd07054..a81d2850a 100644 --- a/src/output/plugins/httpd/IcyMetaDataServer.cxx +++ b/src/output/plugins/httpd/IcyMetaDataServer.cxx @@ -113,5 +113,5 @@ icy_server_metadata_page(const Tag &tag, const TagType *types) if (icy_string.IsNull()) return nullptr; - return Page::Copy(icy_string.c_str(), (icy_string[0] * 16) + 1); + return Page::Copy(icy_string.c_str(), uint8_t(icy_string[0]) * 16 + 1); } From f3b788703e4cc728a415008b0ddd70d1016a09cc Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sun, 19 Feb 2017 19:34:13 +0100 Subject: [PATCH 6/6] tag/Handler: improve snprintf() return value check --- src/tag/TagHandler.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tag/TagHandler.cxx b/src/tag/TagHandler.cxx index 46e3e79b4..345c64a21 100644 --- a/src/tag/TagHandler.cxx +++ b/src/tag/TagHandler.cxx @@ -44,7 +44,7 @@ add_tag_tag(TagType type, const char *value, void *ctx) unsigned n = strtoul(value, &end, 10); if (value != end) { char s[21]; - if (snprintf(s, 21, "%u", n) >= 0) + if (snprintf(s, 21, "%u", n) > 0) tag.AddItem(type, s); } } else