client: fix memory leak in Client::CRF_iofinish::complete

Commit 1210ddf7a ("Client: Add non-blocking helper classes") introduced
Client::C_Read_Finisher Context object for async READ operations, but
it has a read-after-free bug which may cause memory leak when calling
libcephf's non-blocking ceph_ll_nonblocking_readv_writev API with async
READ:

ceph_ll_nonblocking_readv_writev (READ)
  Client::ll_preadv_pwritev
  ...
    Client::_read_async
      Context::complete
        Client::CRF_iofinish::complete
          Client::CRF_iofinish::finish
          CRF->finish_io()
            Client::C_Read_Finisher::finish_io
            ...
            delete this; // frees CRF_iofinish->CRF
          if (CRF->iofinished) // use-after-free of CRF
            delete this; // may not get here

A possible memory leak depends on timing and race with other thread
allocation which alters the memory address of CRF->iofinished to
false, thus skipping the last delete operation.

The check of `if (CRF->iofinished)` is unnecessary: it is always set to
true upon calling CRF->finish_io(). Thus, there is no need to have the
override function Client::CRF_iofinish::complete() as it now has the
same logic as Context::complete(). Removed.

Signed-off-by: Shachar Sharon <ssharon@redhat.com>

diff --git a/src/client/Client.h b/src/client/Client.h
index 08485c844c47a423820d4a169e7c0debd677a667..e1ee5714ef0c9a81eacad55ebb80b4ef4cc4ae00 100644 (file)
--- a/src/client/Client.h
+++ b/src/client/Client.h
@@ -1389,14 +1389,6 @@ private:
     void finish(int r) override {
       CRF->finish_io(r);
     }
-
-    // For _read_async, we may not finish in one go, so be prepared for multiple
-    // calls to complete. All the handling though is in C_Read_Finisher.
-    void complete(int r) override {
-      finish(r);
-      if (CRF->iofinished)
-        delete this;
-    }
   };
 
   class C_Read_Sync_NonBlocking : public Context {