From f76dd085b24ca487930c9c59637b1a5ab60cc035 Mon Sep 17 00:00:00 2001
From: Alex Balgavy <8124851+thezeroalpha@users.noreply.github.com>
Date: Wed, 24 Jul 2024 17:57:38 +0200
Subject: [PATCH] Fix crash when background-fetching lyrics for streams (#561)

When `fetch_lyrics_for_current_song_in_background` is set to `yes`, and
an HTTP(S) stream is loaded with a long URL, ncmpcpp crashes due to the
length of the filename generated to store lyrics. This commit makes the
background lyrics fetcher ignore such streams, preventing the crash.
Addresses issue #380.
---
 src/screens/lyrics.cpp | 26 ++++++++++++++++++++++++--
 src/song.cpp           |  6 ++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/screens/lyrics.cpp b/src/screens/lyrics.cpp
index d34ba617..5ca42869 100644
--- a/src/screens/lyrics.cpp
+++ b/src/screens/lyrics.cpp
@@ -361,6 +361,25 @@ void Lyrics::toggleFetcher()
 		Statusbar::print("Using all lyrics fetchers");
 }
 
+/* For HTTP(S) streams, fetchInBackground makes ncmpcpp crash: the lyrics_file
+ * gets set to the stream URL, which is too long, hence
+ * boost::filesystem::exists() fails.
+ * Possible solutions:
+ * - truncate the URL and use that as a filename. Problem: resulting filename
+ *   might not be unique.
+ * - generate filenames in a different way for streams. Problem: what is a good
+ *   method for this? Perhaps hashing -- but then the lyrics filenames are not
+ *   informative.
+ * - skip fetching lyrics for streams with URLs that are too long. Problem:
+ *   this leads to inconsistent behavior since lyrics will be fetched for some
+ *   streams but not others.
+ * - avoid fetching lyrics for streams altogether.
+ *
+ * We choose the last solution, and ignore streams when fetching lyrics. This
+ * is because fetching lyrics for a stream may not be accurate (streams do not
+ * always provide the necessary metadata to look up lyrics reliably).
+ * Furthermore, fetching lyrics for streams is not necessarily desirable.
+ */
 void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_)
 {
 	auto consumer_impl = [this] {
@@ -377,7 +396,10 @@ void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_)
 					break;
 				}
 				lyrics_file = lyricsFilename(consumer->songs.front().song());
-				if (!boost::filesystem::exists(lyrics_file))
+
+				// For long filenames (e.g. http(s) stream URLs), boost::filesystem::exists() fails.
+				// This if condition is fine, because evaluation order is guaranteed.
+				if (!consumer->songs.front().song().isStream() && !boost::filesystem::exists(lyrics_file))
 				{
 					cs = consumer->songs.front();
 					if (cs.notify())
@@ -389,7 +411,7 @@ void Lyrics::fetchInBackground(const MPD::Song &s, bool notify_)
 				}
 				consumer->songs.pop();
 			}
-			if (!cs.song().empty())
+			if (!cs.song().empty() && !cs.song().isStream())
 			{
 				auto lyrics = downloadLyrics(cs.song(), nullptr, nullptr, m_fetcher);
 				if (lyrics)
diff --git a/src/song.cpp b/src/song.cpp
index 70822d10..c274d43f 100644
--- a/src/song.cpp
+++ b/src/song.cpp
@@ -293,7 +293,9 @@ bool Song::isFromDatabase() const
 bool Song::isStream() const
 {
 	assert(m_song);
-	return !strncmp(mpd_song_get_uri(m_song.get()), "http://", 7);
+	const char *song_uri = mpd_song_get_uri(m_song.get());
+	// Stream schemas: http, https
+	return !strncmp(song_uri, "http://", 7) || !strncmp(song_uri, "https://", 8);
 }
 
 bool Song::empty() const
@@ -308,7 +310,7 @@ std::string Song::ShowTime(unsigned length)
 	int minutes = length/60;
 	length -= minutes*60;
 	int seconds = length;
-	
+
 	std::string result;
 	if (hours > 0)
 		result = (boost::format("%d:%02d:%02d") % hours % minutes % seconds).str();