lib/nfs/Manager: defer NfsConnection destruction

Avoids a crash that occurs when NfsConnection::OnSocketReady()
dereferences itself before returning.

3 files changed, 56 insertions, 9 deletions

diff --git a/NEWS b/NEWS
index 7d04d528c..5d089033a 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,6 @@
 ver 0.19.5 (not yet released)
+* input
+  - nfs: fix crash on connection failure
 * decoder
   - dsdiff, dsf, opus: fix deadlock while seeking
   - mp4v2: remove because of incompatible license
diff --git a/src/lib/nfs/Manager.cxx b/src/lib/nfs/Manager.cxx
index c5aecf48d..6d50cce18 100644
--- a/src/lib/nfs/Manager.cxx
+++ b/src/lib/nfs/Manager.cxx
@@ -29,8 +29,10 @@ NfsManager::ManagedConnection::OnNfsConnectionError(Error &&error)
 {
 	FormatError(error, "NFS error on %s:%s", GetServer(), GetExportName());
 
-	manager.connections.erase(manager.connections.iterator_to(*this));
-	delete this;
+	/* defer deletion so the caller
+	   (i.e. NfsConnection::OnSocketReady()) can still use this
+	   object */
+	manager.ScheduleDelete(*this);
 }
 
 inline bool
@@ -59,7 +61,9 @@ NfsManager::Compare::operator()(const ManagedConnection &a,
 
 NfsManager::~NfsManager()
 {
-	assert(loop.IsInside());
+	assert(GetEventLoop().IsInside());
+
+	CollectGarbage();
 
 	connections.clear_and_dispose([](ManagedConnection *c){
 			delete c;
@@ -71,13 +75,13 @@ NfsManager::GetConnection(const char *server, const char *export_name)
 {
 	assert(server != nullptr);
 	assert(export_name != nullptr);
-	assert(loop.IsInside());
+	assert(GetEventLoop().IsInside());
 
 	Map::insert_commit_data hint;
 	auto result = connections.insert_check(LookupKey{server, export_name},
 					       Compare(), hint);
 	if (result.second) {
-		auto c = new ManagedConnection(*this, loop,
+		auto c = new ManagedConnection(*this, GetEventLoop(),
 					       server, export_name);
 		connections.insert_commit(*c, hint);
 		return *c;
@@ -85,3 +89,19 @@ NfsManager::GetConnection(const char *server, const char *export_name)
 		return *result.first;
 	}
 }
+
+void
+NfsManager::CollectGarbage()
+{
+	assert(GetEventLoop().IsInside());
+
+	garbage.clear_and_dispose([](ManagedConnection *c){
+			delete c;
+		});
+}
+
+void
+NfsManager::OnIdle()
+{
+	CollectGarbage();
+}
diff --git a/src/lib/nfs/Manager.hxx b/src/lib/nfs/Manager.hxx
index 612b01f9c..130c81aca 100644
--- a/src/lib/nfs/Manager.hxx
+++ b/src/lib/nfs/Manager.hxx
@@ -23,14 +23,16 @@
 #include "check.h"
 #include "Connection.hxx"
 #include "Compiler.h"
+#include "event/IdleMonitor.hxx"
 
 #include <boost/intrusive/set.hpp>
+#include <boost/intrusive/slist.hpp>
 
 /**
  * A manager for NFS connections.  Handles multiple connections to
  * multiple NFS servers.
  */
-class NfsManager {
+class NfsManager final : IdleMonitor {
 	struct LookupKey {
 		const char *server;
 		const char *export_name;
@@ -38,6 +40,7 @@ class NfsManager {
 
 	class ManagedConnection final
 		: public NfsConnection,
+		  public boost::intrusive::slist_base_hook<boost::intrusive::link_mode<boost::intrusive::normal_link>>,
 		  public boost::intrusive::set_base_hook<boost::intrusive::link_mode<boost::intrusive::normal_link>> {
 		NfsManager &manager;
 
@@ -63,8 +66,6 @@ class NfsManager {
 				const LookupKey b) const;
 	};
 
-	EventLoop &loop;
-
 	/**
 	 * Maps server and export_name to #ManagedConnection.
 	 */
@@ -74,9 +75,18 @@ class NfsManager {
 
 	Map connections;
 
+	typedef boost::intrusive::slist<ManagedConnection> List;
+
+	/**
+	 * A list of "garbage" connection objects.  Their destruction
+	 * is postponed because they were thrown into the garbage list
+	 * when callers on the stack were still using them.
+	 */
+	List garbage;
+
 public:
 	NfsManager(EventLoop &_loop)
-		:loop(_loop) {}
+		:IdleMonitor(_loop) {}
 
 	/**
 	 * Must be run from EventLoop's thread.
@@ -86,6 +96,21 @@ public:
 	gcc_pure
 	NfsConnection &GetConnection(const char *server,
 				     const char *export_name);
+
+private:
+	void ScheduleDelete(ManagedConnection &c) {
+		connections.erase(connections.iterator_to(c));
+		garbage.push_front(c);
+		IdleMonitor::Schedule();
+	}
+
+	/**
+	 * Delete all connections on the #garbage list.
+	 */
+	void CollectGarbage();
+
+	/* virtual methods from IdleMonitor */
+	void OnIdle() override;
 };
 
 #endif