client: fix segment fault in SourceReader

After #172 and #209, source volume data of the clone volume
can be read within the client through SourceReader, and each
source volume was also opened and represented by a FileInstance,
but `FileInstance::mdsclient_` points to `FileClient::mdsClient_`,
so after FileClient is destroyed, `FileInstance::mdsclient_`
becomes dangling pointer.

To fix this problem, we let `FileClient::mdsClient_` be a shared_ptr,
and each FileInstance holds ownership of it.

Signed-off-by: Hanqing Wu <wuhanqing@corp.netease.com>

src/client/client_metric.h

@@ -30,6 +30,7 @@
 
 #include "src/common/timeutility.h"
 #include "src/client/client_common.h"
+#include "src/common/string_util.h"
 
 using curve::common::TimeUtility;
 
@@ -200,8 +201,7 @@ struct MDSClientMetric {
     explicit MDSClientMetric(const std::string& prefix_ = "")
         : prefix(!prefix_.empty()
                      ? prefix_
-                     : "curve_mds_client_" +
-                           std::to_string(reinterpret_cast<uint64_t>(this))),
+                     : "curve_mds_client_" + common::ToHexString(this)),
           metaserverAddress(prefix, "current_metaserver_addr", GetStringValue,
                             &metaserverAddr),
           openFile(prefix, "openFile"),

src/client/file_instance.cpp

@@ -24,6 +24,7 @@
 
 #include <butil/endpoint.h>
 #include <glog/logging.h>
+#include <utility>
 
 #include "proto/nameserver2.pb.h"
 #include "proto/topology.pb.h"
@@ -50,7 +51,7 @@ FileInstance::FileInstance()
       readonly_(false) {}
 
 bool FileInstance::Initialize(const std::string& filename,
-                              MDSClient* mdsclient,
+                              std::shared_ptr<MDSClient> mdsclient,
                               const UserInfo_t& userinfo,
                               const FileServiceOption& fileservicopt,
                               bool readonly) {
@@ -69,11 +70,12 @@ bool FileInstance::Initialize(const std::string& filename,
         }
 
         finfo_.userinfo = userinfo;
-        mdsclient_ = mdsclient;
+        mdsclient_ = std::move(mdsclient);
 
         finfo_.fullPathName = filename;
 
-        if (!iomanager4file_.Initialize(filename, fileopt_.ioOpt, mdsclient_)) {
+        if (!iomanager4file_.Initialize(filename, fileopt_.ioOpt,
+                                        mdsclient_.get())) {
             LOG(ERROR) << "Init io context manager failed, filename = "
                        << filename;
             break;
@@ -82,7 +84,8 @@ bool FileInstance::Initialize(const std::string& filename,
         iomanager4file_.UpdateFileInfo(finfo_);
 
         leaseExecutor_.reset(new (std::nothrow) LeaseExecutor(
-            fileopt_.leaseOpt, finfo_.userinfo, mdsclient_, &iomanager4file_));
+            fileopt_.leaseOpt, finfo_.userinfo, mdsclient_.get(),
+            &iomanager4file_));
         if (CURVE_UNLIKELY(leaseExecutor_ == nullptr)) {
             LOG(ERROR) << "Allocate LeaseExecutor failed, filename = "
                        << filename;
@@ -97,35 +100,38 @@ bool FileInstance::Initialize(const std::string& filename,
 
 void FileInstance::UnInitialize() {
     iomanager4file_.UnInitialize();
+
+    // release the ownership of mdsclient
+    mdsclient_.reset();
 }
 
 int FileInstance::Read(char* buf, off_t offset, size_t length) {
-    return iomanager4file_.Read(buf, offset, length, mdsclient_);
+    return iomanager4file_.Read(buf, offset, length, mdsclient_.get());
 }
 
 int FileInstance::Write(const char* buf, off_t offset, size_t len) {
     if (readonly_) {
         DVLOG(9) << "open with read only, do not support write!";
         return -1;
     }
-    return iomanager4file_.Write(buf, offset, len, mdsclient_);
+    return iomanager4file_.Write(buf, offset, len, mdsclient_.get());
 }
 
 int FileInstance::AioRead(CurveAioContext* aioctx, UserDataType dataType) {
-    return iomanager4file_.AioRead(aioctx, mdsclient_, dataType);
+    return iomanager4file_.AioRead(aioctx, mdsclient_.get(), dataType);
 }
 
 int FileInstance::AioWrite(CurveAioContext* aioctx, UserDataType dataType) {
     if (readonly_) {
         DVLOG(9) << "open with read only, do not support write!";
         return -1;
     }
-    return iomanager4file_.AioWrite(aioctx, mdsclient_, dataType);
+    return iomanager4file_.AioWrite(aioctx, mdsclient_.get(), dataType);
 }
 
 int FileInstance::Discard(off_t offset, size_t length) {
     if (!readonly_) {
-        return iomanager4file_.Discard(offset, length, mdsclient_);
+        return iomanager4file_.Discard(offset, length, mdsclient_.get());
     }
 
     LOG(ERROR) << "Open with read only, not support Discard";
@@ -134,7 +140,7 @@ int FileInstance::Discard(off_t offset, size_t length) {
 
 int FileInstance::AioDiscard(CurveAioContext* aioctx) {
     if (!readonly_) {
-        return iomanager4file_.AioDiscard(aioctx, mdsclient_);
+        return iomanager4file_.AioDiscard(aioctx, mdsclient_.get());
     }
 
     LOG(ERROR) << "Open with read only, not support AioDiscard";
@@ -197,7 +203,7 @@ int FileInstance::Close() {
 
 FileInstance* FileInstance::NewInitedFileInstance(
     const FileServiceOption& fileServiceOption,
-    MDSClient* mdsClient,
+    std::shared_ptr<MDSClient> mdsClient,
     const std::string& filename,
     const UserInfo& userInfo,
     bool readonly) {
@@ -207,7 +213,7 @@ FileInstance* FileInstance::NewInitedFileInstance(
         return nullptr;
     }
 
-    bool ret = instance->Initialize(filename, mdsClient, userInfo,
+    bool ret = instance->Initialize(filename, std::move(mdsClient), userInfo,
                                     fileServiceOption, readonly);
     if (!ret) {
         LOG(ERROR) << "FileInstance initialize failed"
@@ -222,11 +228,11 @@ FileInstance* FileInstance::NewInitedFileInstance(
 }
 
 FileInstance* FileInstance::Open4Readonly(const FileServiceOption& opt,
-                                          MDSClient* mdsclient,
+                                          std::shared_ptr<MDSClient> mdsclient,
                                           const std::string& filename,
                                           const UserInfo& userInfo) {
     FileInstance* instance = FileInstance::NewInitedFileInstance(
-        opt, mdsclient, filename, userInfo, true);
+        opt, std::move(mdsclient), filename, userInfo, true);
     if (instance == nullptr) {
         LOG(ERROR) << "NewInitedFileInstance failed, filename = " << filename;
         return nullptr;

src/client/file_instance.h

@@ -52,7 +52,7 @@ class CURVE_CACHELINE_ALIGNMENT FileInstance {
      * @return: 成功返回true、否则返回false
      */
     bool Initialize(const std::string& filename,
-                    MDSClient* mdsclient,
+                    std::shared_ptr<MDSClient> mdsclient,
                     const UserInfo_t& userinfo,
                     const FileServiceOption& fileservicopt,
                     bool readonly = false);
@@ -152,13 +152,13 @@ class CURVE_CACHELINE_ALIGNMENT FileInstance {
 
     static FileInstance* NewInitedFileInstance(
         const FileServiceOption& fileServiceOption,
-        MDSClient* mdsClient,
+        std::shared_ptr<MDSClient> mdsClient,
         const std::string& filename,
         const UserInfo& userInfo,
         bool readonly);
 
     static FileInstance* Open4Readonly(const FileServiceOption& opt,
-                                       MDSClient* mdsclient,
+                                       std::shared_ptr<MDSClient> mdsclient,
                                        const std::string& filename,
                                        const UserInfo& userInfo);
 
@@ -170,7 +170,7 @@ class CURVE_CACHELINE_ALIGNMENT FileInstance {
     FileServiceOption       fileopt_;
 
     // MDSClient是FileInstance与mds通信的唯一出口
-    MDSClient*              mdsclient_;
+    std::shared_ptr<MDSClient> mdsclient_;
 
     // 每个文件都持有与MDS通信的lease，LeaseExecutor是续约执行者
     std::unique_ptr<LeaseExecutor> leaseExecutor_;

src/client/libcurve_file.cpp

@@ -29,6 +29,7 @@
 #include <memory>
 #include <mutex>   // NOLINT
 #include <thread>  // NOLINT
+#include <utility>
 
 #include "include/client/libcurve.h"
 #include "include/curve_compiler_specific.h"
@@ -42,6 +43,7 @@
 #include "src/common/curve_version.h"
 #include "src/common/net_common.h"
 #include "src/common/uuid.h"
+#include "src/common/string_util.h"
 
 #define PORT_LIMIT  65535
 
@@ -96,11 +98,14 @@ void InitLogging(const std::string& confPath) {
     static LoggerGuard guard(confPath);
 }
 
-FileClient::FileClient(): fdcount_(0), openedFileNum_("opened_file_num") {
-    inited_ = false;
-    mdsClient_ = nullptr;
-    fileserviceMap_.clear();
-}
+FileClient::FileClient()
+    : rwlock_(),
+      fdcount_(0),
+      fileserviceMap_(),
+      clientconfig_(),
+      mdsClient_(),
+      inited_(false),
+      openedFileNum_(common::ToHexString(this)) {}
 
 int FileClient::Init(const std::string& configpath) {
     if (inited_) {
@@ -116,17 +121,13 @@ int FileClient::Init(const std::string& configpath) {
         return -LIBCURVE_ERROR::FAILED;
     }
 
-    mdsClient_ = new (std::nothrow) MDSClient();
-    if (mdsClient_ == nullptr) {
-        return -LIBCURVE_ERROR::FAILED;
-    }
 
-    auto ret = mdsClient_->Initialize(clientconfig_.
-               GetFileServiceOption().metaServerOpt);
+    auto tmpMdsClient = std::make_shared<MDSClient>();
+
+    auto ret = tmpMdsClient->Initialize(
+        clientconfig_.GetFileServiceOption().metaServerOpt);
     if (LIBCURVE_ERROR::OK != ret) {
         LOG(ERROR) << "Init global mds client failed!";
-        delete mdsClient_;
-        mdsClient_ = nullptr;
         return -LIBCURVE_ERROR::FAILED;
     }
 
@@ -139,12 +140,10 @@ int FileClient::Init(const std::string& configpath) {
 
     bool rc = StartDummyServer();
     if (rc == false) {
-        mdsClient_->UnInitialize();
-        delete mdsClient_;
-        mdsClient_ = nullptr;
         return -LIBCURVE_ERROR::FAILED;
     }
 
+    mdsClient_ = std::move(tmpMdsClient);
     inited_ = true;
     return LIBCURVE_ERROR::OK;
 }
@@ -161,11 +160,7 @@ void FileClient::UnInit() {
     }
     fileserviceMap_.clear();
 
-    if (mdsClient_ != nullptr) {
-        mdsClient_->UnInitialize();
-        delete mdsClient_;
-        mdsClient_ = nullptr;
-    }
+    mdsClient_.reset();
     inited_ = false;
 }
 

src/client/libcurve_file.h

@@ -28,6 +28,7 @@
 #include <string>
 #include <unordered_map>
 #include <vector>
+#include <memory>
 
 #include "include/client/libcurve.h"
 #include "src/client/client_common.h"
@@ -314,31 +315,6 @@ class FileClient {
         return openedFileNum_.get_value();
     }
 
-    /**
-     * test use, set the mdsclient_
-     */
-    void SetMdsClient(MDSClient* client) {
-        mdsClient_ = client;
-    }
-
-    /**
-     * test use, set the clientconfig_
-     */
-    void SetClientConfig(ClientConfig cfg) {
-        clientconfig_ = cfg;
-    }
-
-    const ClientConfig& GetClientConfig() {
-        return clientconfig_;
-    }
-
-    /**
-     * test use, get the fileserviceMap_
-     */
-    std::unordered_map<int, FileInstance*>& GetFileServiceMap() {
-        return fileserviceMap_;
-    }
-
  private:
     bool StartDummyServer();
 
@@ -360,7 +336,7 @@ class FileClient {
     ClientConfig clientconfig_;
 
     // fileclient对应的全局mdsclient
-    MDSClient* mdsClient_;
+    std::shared_ptr<MDSClient> mdsClient_;
 
     // 是否初始化成功
     bool inited_;