From 33ee35ab928fab752aa7f533e6a9a2b46de6528a Mon Sep 17 00:00:00 2001
From: Max Kellermann <max@musicpd.org>
Date: Mon, 6 Jul 2020 17:45:59 +0200
Subject: [PATCH] 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> storage)
 	const std::lock_guard<Mutex> 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;
 }