From 0661fd6f7c66ae888b6fc253af6dd0514798eff5 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 26 Sep 2014 13:29:44 +0200 Subject: 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(). --- src/lib/nfs/Cancellable.hxx | 7 +++++++ src/lib/nfs/Connection.cxx | 43 ++++++++++++++++++++++++++++++++++++++++++- src/lib/nfs/Connection.hxx | 33 ++++++++++++++++++++++++++++++++- src/lib/nfs/FileReader.cxx | 15 +++++++++++---- 4 files changed, 92 insertions(+), 6 deletions(-) (limited to 'src/lib/nfs') 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 @@ -80,10 +80,23 @@ NfsConnection::CancellableCallback::Read(nfs_context *ctx, struct nfsfh *fh, return true; } +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(); @@ -224,6 +241,13 @@ NfsConnection::Close(struct nfsfh *fh) ScheduleSocket(); } +void +NfsConnection::CancelAndClose(struct nfsfh *fh, NfsCallback &callback) +{ + CancellableCallback &cancel = callbacks.Get(callback); + cancel.CancelAndScheduleClose(fh); +} + void NfsConnection::DestroyContext() { @@ -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 + #include #include +#include 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(_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 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 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; } -- cgit v1.2.3