lib/nfs/FileReader: postpone the nfs_close_async() call

If an async opertion is in progress, nfs_close_async() will make
libnfs crash because the RPC callback will dereference an object that
was freed by nfs_close_async().

4 files changed, 92 insertions, 6 deletions

diff --git a/src/lib/nfs/Cancellable.hxx b/src/lib/nfs/Cancellable.hxx
index 1f287c329..be4527ac3 100644
--- a/src/lib/nfs/Cancellable.hxx
+++ b/src/lib/nfs/Cancellable.hxx
@@ -150,6 +150,13 @@ public:
 
 		i->Cancel();
 	}
+
+	CT &Get(reference_type p) {
+		auto i = Find(p);
+		assert(i != list.end());
+
+		return *i;
+	}
 };
 
 #endif
diff --git a/src/lib/nfs/Connection.cxx b/src/lib/nfs/Connection.cxx
index 853cacc9a..e5dc87727 100644
--- a/src/lib/nfs/Connection.cxx
+++ b/src/lib/nfs/Connection.cxx
@@ -81,9 +81,22 @@ NfsConnection::CancellableCallback::Read(nfs_context *ctx, struct nfsfh *fh,
 }
 
 inline void
+NfsConnection::CancellableCallback::CancelAndScheduleClose(struct nfsfh *fh)
+{
+	assert(!open);
+	assert(close_fh == nullptr);
+	assert(fh != nullptr);
+
+	close_fh = fh;
+	Cancel();
+}
+
+inline void
 NfsConnection::CancellableCallback::Callback(int err, void *data)
 {
 	if (!IsCancelled()) {
+		assert(close_fh == nullptr);
+
 		NfsCallback &cb = Get();
 
 		connection.callbacks.Remove(*this);
@@ -98,9 +111,12 @@ NfsConnection::CancellableCallback::Callback(int err, void *data)
 			/* a nfs_open_async() call was cancelled - to
 			   avoid a memory leak, close the newly
 			   allocated file handle immediately */
+			assert(close_fh == nullptr);
+
 			struct nfsfh *fh = (struct nfsfh *)data;
 			connection.Close(fh);
-		}
+		} else if (close_fh != nullptr)
+			connection.DeferClose(close_fh);
 
 		connection.callbacks.Remove(*this);
 	}
@@ -135,6 +151,7 @@ NfsConnection::~NfsConnection()
 	assert(new_leases.empty());
 	assert(active_leases.empty());
 	assert(callbacks.IsEmpty());
+	assert(deferred_close.empty());
 
 	if (context != nullptr)
 		DestroyContext();
@@ -225,6 +242,13 @@ NfsConnection::Close(struct nfsfh *fh)
 }
 
 void
+NfsConnection::CancelAndClose(struct nfsfh *fh, NfsCallback &callback)
+{
+	CancellableCallback &cancel = callbacks.Get(callback);
+	cancel.CancelAndScheduleClose(fh);
+}
+
+void
 NfsConnection::DestroyContext()
 {
 	assert(GetEventLoop().IsInside());
@@ -237,6 +261,15 @@ NfsConnection::DestroyContext()
 	context = nullptr;
 }
 
+inline void
+NfsConnection::DeferClose(struct nfsfh *fh)
+{
+	assert(in_event);
+	assert(in_service);
+
+	deferred_close.push_front(fh);
+}
+
 void
 NfsConnection::ScheduleSocket()
 {
@@ -257,6 +290,8 @@ NfsConnection::ScheduleSocket()
 bool
 NfsConnection::OnSocketReady(unsigned flags)
 {
+	assert(deferred_close.empty());
+
 	bool closed = false;
 
 	const bool was_mounted = mount_finished;
@@ -278,6 +313,12 @@ NfsConnection::OnSocketReady(unsigned flags)
 	assert(in_service);
 	in_service = false;
 
+	while (!deferred_close.empty()) {
+		nfs_close_async(context, deferred_close.front(),
+				DummyCallback, nullptr);
+		deferred_close.pop_front();
+	}
+
 	if (!was_mounted && mount_finished) {
 		if (postponed_mount_error.IsDefined()) {
 			DestroyContext();
diff --git a/src/lib/nfs/Connection.hxx b/src/lib/nfs/Connection.hxx
index 880b7d467..b3db37c5d 100644
--- a/src/lib/nfs/Connection.hxx
+++ b/src/lib/nfs/Connection.hxx
@@ -26,8 +26,11 @@
 #include "event/DeferredMonitor.hxx"
 #include "util/Error.hxx"
 
+#include <boost/intrusive/list.hpp>
+
 #include <string>
 #include <list>
+#include <forward_list>
 
 struct nfs_context;
 class NfsCallback;
@@ -47,13 +50,19 @@ class NfsConnection : SocketMonitor, DeferredMonitor {
 		 */
 		const bool open;
 
+		/**
+		 * The file handle scheduled to be closed as soon as
+		 * the operation finishes.
+		 */
+		struct nfsfh *close_fh;
+
 	public:
 		explicit CancellableCallback(NfsCallback &_callback,
 					     NfsConnection &_connection,
 					     bool _open)
 			:CancellablePointer<NfsCallback>(_callback),
 			 connection(_connection),
-			 open(_open) {}
+			 open(_open), close_fh(nullptr) {}
 
 		bool Open(nfs_context *context, const char *path, int flags,
 			  Error &error);
@@ -63,6 +72,12 @@ class NfsConnection : SocketMonitor, DeferredMonitor {
 			  uint64_t offset, size_t size,
 			  Error &error);
 
+		/**
+		 * Cancel the operation and schedule a call to
+		 * nfs_close_async() with the given file handle.
+		 */
+		void CancelAndScheduleClose(struct nfsfh *fh);
+
 	private:
 		static void Callback(int err, struct nfs_context *nfs,
 				     void *data, void *private_data);
@@ -79,6 +94,15 @@ class NfsConnection : SocketMonitor, DeferredMonitor {
 	typedef CancellableList<NfsCallback, CancellableCallback> CallbackList;
 	CallbackList callbacks;
 
+	/**
+	 * A list of NFS file handles (struct nfsfh *) which shall be
+	 * closed as soon as nfs_service() returns.  If we close the
+	 * file handle while in nfs_service(), libnfs may crash, and
+	 * deferring this call to after nfs_service() avoids this
+	 * problem.
+	 */
+	std::forward_list<struct nfsfh *> deferred_close;
+
 	Error postponed_mount_error;
 
 	/**
@@ -135,6 +159,7 @@ public:
 	void Cancel(NfsCallback &callback);
 
 	void Close(struct nfsfh *fh);
+	void CancelAndClose(struct nfsfh *fh, NfsCallback &callback);
 
 protected:
 	virtual void OnNfsConnectionError(Error &&error) = 0;
@@ -145,6 +170,12 @@ private:
 	}
 
 	void DestroyContext();
+
+	/**
+	 * Invoke nfs_close_async() after nfs_service() returns.
+	 */
+	void DeferClose(struct nfsfh *fh);
+
 	bool MountInternal(Error &error);
 	void BroadcastMountSuccess();
 	void BroadcastMountError(Error &&error);
diff --git a/src/lib/nfs/FileReader.cxx b/src/lib/nfs/FileReader.cxx
index 52d951fa6..d2be46f8e 100644
--- a/src/lib/nfs/FileReader.cxx
+++ b/src/lib/nfs/FileReader.cxx
@@ -57,11 +57,18 @@ NfsFileReader::Close()
 
 	connection->RemoveLease(*this);
 
-	if (state > State::MOUNT && state != State::IDLE)
-		connection->Cancel(*this);
-
-	if (state > State::OPEN)
+	if (state == State::IDLE)
+		/* no async operation in progress: can close
+		   immediately */
 		connection->Close(fh);
+	else if (state > State::OPEN)
+		/* one async operation in progress: cancel it and
+		   defer the nfs_close_async() call */
+		connection->CancelAndClose(fh, *this);
+	else if (state > State::MOUNT)
+		/* we don't have a file handle yet - just cancel the
+		   async operation */
+		connection->Cancel(*this);
 
 	state = State::INITIAL;
 }