Merge pull request #21686 from kitaisreal/library-dictionary-source-fix-possible-leak

LibraryDictionarySource fix possible leak

src/Common/SharedLibrary.cpp

@@ -13,11 +13,11 @@ namespace ErrorCodes
     extern const int CANNOT_DLSYM;
 }
 
-SharedLibrary::SharedLibrary(const std::string & path, int flags)
+SharedLibrary::SharedLibrary(std::string_view path, int flags)
 {
-    handle = dlopen(path.c_str(), flags);
+    handle = dlopen(path.data(), flags);
     if (!handle)
-        throw Exception(std::string("Cannot dlopen: ") + dlerror(), ErrorCodes::CANNOT_DLOPEN);
+        throw Exception(ErrorCodes::CANNOT_DLOPEN, "Cannot dlopen: ({})", dlerror());
 
     updatePHDRCache();
 
@@ -31,17 +31,18 @@ SharedLibrary::~SharedLibrary()
         std::terminate();
 }
 
-void * SharedLibrary::getImpl(const std::string & name, bool no_throw)
+void * SharedLibrary::getImpl(std::string_view name, bool no_throw)
 {
     dlerror();
 
-    auto * res = dlsym(handle, name.c_str());
+    auto * res = dlsym(handle, name.data());
 
     if (char * error = dlerror())
     {
         if (no_throw)
             return nullptr;
-        throw Exception(std::string("Cannot dlsym: ") + error, ErrorCodes::CANNOT_DLSYM);
+
+        throw Exception(ErrorCodes::CANNOT_DLSYM, "Cannot dlsym: ({})", error);
     }
 
     return res;

src/Common/SharedLibrary.h

@@ -14,23 +14,24 @@ namespace DB
 class SharedLibrary : private boost::noncopyable
 {
 public:
-    explicit SharedLibrary(const std::string & path, int flags = RTLD_LAZY);
+    explicit SharedLibrary(std::string_view path, int flags = RTLD_LAZY);
 
     ~SharedLibrary();
 
     template <typename Func>
-    Func get(const std::string & name)
+    Func get(std::string_view name)
     {
         return reinterpret_cast<Func>(getImpl(name));
     }
+
     template <typename Func>
-    Func tryGet(const std::string & name)
+    Func tryGet(std::string_view name)
     {
         return reinterpret_cast<Func>(getImpl(name, true));
     }
 
 private:
-    void * getImpl(const std::string & name, bool no_throw = false);
+    void * getImpl(std::string_view name, bool no_throw = false);
 
     void * handle = nullptr;
 };

src/Dictionaries/LibraryDictionarySource.cpp

@@ -72,7 +72,7 @@ namespace
     }
 
 
-    Block dataToBlock(const Block & sample_block, const void * data)
+    Block dataToBlock(const Block & sample_block, const ClickHouseLibrary::RawClickHouseLibraryTable data)
     {
         if (!data)
             throw Exception("LibraryDictionarySource: No data returned", ErrorCodes::EXTERNAL_LIBRARY_ERROR);
@@ -84,9 +84,7 @@ namespace
                     + (columns_received->error_string ? columns_received->error_string : ""),
                 ErrorCodes::EXTERNAL_LIBRARY_ERROR);
 
-        MutableColumns columns(sample_block.columns());
-        for (const auto i : ext::range(0, columns.size()))
-            columns[i] = sample_block.getByPosition(i).column->cloneEmpty();
+        MutableColumns columns = sample_block.cloneEmptyColumns();
 
         for (size_t col_n = 0; col_n < columns_received->size; ++col_n)
         {
@@ -151,8 +149,8 @@ LibraryDictionarySource::LibraryDictionarySource(
 #endif
     );
     settings = std::make_shared<CStringsHolder>(getLibSettings(config, config_prefix + lib_config_settings));
-    if (auto lib_new = library->tryGet<decltype(lib_data) (*)(decltype(&settings->strings), decltype(&ClickHouseLibrary::log))>(
-            "ClickHouseDictionary_v3_libNew"))
+
+    if (auto lib_new = library->tryGet<ClickHouseLibrary::LibraryNewFunc>(ClickHouseLibrary::LIBRARY_CREATE_NEW_FUNC_NAME))
         lib_data = lib_new(&settings->strings, ClickHouseLibrary::log);
 }
 
@@ -166,17 +164,15 @@ LibraryDictionarySource::LibraryDictionarySource(const LibraryDictionarySource &
     , description{other.description}
     , settings{other.settings}
 {
-    if (auto lib_clone = library->tryGet<decltype(lib_data) (*)(decltype(other.lib_data))>("ClickHouseDictionary_v3_libClone"))
+    if (auto lib_clone = library->tryGet<ClickHouseLibrary::LibraryCloneFunc>(ClickHouseLibrary::LIBRARY_CLONE_FUNC_NAME))
         lib_data = lib_clone(other.lib_data);
-    else if (
-        auto lib_new = library->tryGet<decltype(lib_data) (*)(decltype(&settings->strings), decltype(&ClickHouseLibrary::log))>(
-            "ClickHouseDictionary_v3_libNew"))
+    else if (auto lib_new = library->tryGet<ClickHouseLibrary::LibraryNewFunc>(ClickHouseLibrary::LIBRARY_CREATE_NEW_FUNC_NAME))
         lib_data = lib_new(&settings->strings, ClickHouseLibrary::log);
 }
 
 LibraryDictionarySource::~LibraryDictionarySource()
 {
-    if (auto lib_delete = library->tryGet<void (*)(decltype(lib_data))>("ClickHouseDictionary_v3_libDelete"))
+    if (auto lib_delete = library->tryGet<ClickHouseLibrary::LibraryDeleteFunc>(ClickHouseLibrary::LIBRARY_DELETE_FUNC_NAME))
         lib_delete(lib_data);
 }
 
@@ -193,15 +189,17 @@ BlockInputStreamPtr LibraryDictionarySource::loadAll()
         columns.data[i] = a.name.c_str();
         ++i;
     }
-    void * data_ptr = nullptr;
 
-    /// Get function pointer before dataNew call because library->get may throw.
-    auto func_load_all
-        = library->get<void * (*)(decltype(data_ptr), decltype(&settings->strings), decltype(&columns))>("ClickHouseDictionary_v3_loadAll");
-    data_ptr = library->get<decltype(data_ptr) (*)(decltype(lib_data))>("ClickHouseDictionary_v3_dataNew")(lib_data);
-    auto * data = func_load_all(data_ptr, &settings->strings, &columns);
+    auto load_all_func = library->get<ClickHouseLibrary::LibraryLoadAllFunc>(ClickHouseLibrary::LIBRARY_LOAD_ALL_FUNC_NAME);
+    auto data_new_func = library->get<ClickHouseLibrary::LibraryDataNewFunc>(ClickHouseLibrary::LIBRARY_DATA_NEW_FUNC_NAME);
+    auto data_delete_func = library->get<ClickHouseLibrary::LibraryDataDeleteFunc>(ClickHouseLibrary::LIBRARY_DATA_DELETE_FUNC_NAME);
+
+    ClickHouseLibrary::LibraryData data_ptr = data_new_func(lib_data);
+    SCOPE_EXIT(data_delete_func(lib_data, data_ptr));
+
+    ClickHouseLibrary::RawClickHouseLibraryTable data = load_all_func(data_ptr, &settings->strings, &columns);
     auto block = dataToBlock(description.sample_block, data);
-    SCOPE_EXIT(library->get<void (*)(decltype(lib_data), decltype(data_ptr))>("ClickHouseDictionary_v3_dataDelete")(lib_data, data_ptr));
+
     return std::make_shared<OneBlockInputStream>(block);
 }
 
@@ -219,16 +217,17 @@ BlockInputStreamPtr LibraryDictionarySource::loadIds(const std::vector<UInt64> &
         columns_pass.data[i] = a.name.c_str();
         ++i;
     }
-    void * data_ptr = nullptr;
 
-    /// Get function pointer before dataNew call because library->get may throw.
-    auto func_load_ids
-        = library->get<void * (*)(decltype(data_ptr), decltype(&settings->strings), decltype(&columns_pass), decltype(&ids_data))>(
-            "ClickHouseDictionary_v3_loadIds");
-    data_ptr = library->get<decltype(data_ptr) (*)(decltype(lib_data))>("ClickHouseDictionary_v3_dataNew")(lib_data);
-    auto * data = func_load_ids(data_ptr, &settings->strings, &columns_pass, &ids_data);
+    auto load_ids_func = library->get<ClickHouseLibrary::LibraryLoadIdsFunc>(ClickHouseLibrary::LIBRARY_LOAD_IDS_FUNC_NAME);
+    auto data_new_func = library->get<ClickHouseLibrary::LibraryDataNewFunc>(ClickHouseLibrary::LIBRARY_DATA_NEW_FUNC_NAME);
+    auto data_delete_func = library->get<ClickHouseLibrary::LibraryDataDeleteFunc>(ClickHouseLibrary::LIBRARY_DATA_DELETE_FUNC_NAME);
+
+    ClickHouseLibrary::LibraryData data_ptr = data_new_func(lib_data);
+    SCOPE_EXIT(data_delete_func(lib_data, data_ptr));
+
+    ClickHouseLibrary::RawClickHouseLibraryTable data = load_ids_func(data_ptr, &settings->strings, &columns_pass, &ids_data);
     auto block = dataToBlock(description.sample_block, data);
-    SCOPE_EXIT(library->get<void (*)(decltype(lib_data), decltype(data_ptr))>("ClickHouseDictionary_v3_dataDelete")(lib_data, data_ptr));
+
     return std::make_shared<OneBlockInputStream>(block);
 }
 
@@ -254,30 +253,34 @@ BlockInputStreamPtr LibraryDictionarySource::loadKeys(const Columns & key_column
 
     ClickHouseLibrary::Table request_cols{.data = static_cast<ClickHouseLibrary::Row *>(holder.get()), .size = key_columns.size()};
 
-    void * data_ptr = nullptr;
-    /// Get function pointer before dataNew call because library->get may throw.
-    auto func_load_keys = library->get<void * (*)(decltype(data_ptr), decltype(&settings->strings), decltype(&request_cols))>(
-        "ClickHouseDictionary_v3_loadKeys");
-    data_ptr = library->get<decltype(data_ptr) (*)(decltype(lib_data))>("ClickHouseDictionary_v3_dataNew")(lib_data);
-    auto * data = func_load_keys(data_ptr, &settings->strings, &request_cols);
+    auto load_keys_func = library->get<ClickHouseLibrary::LibraryLoadKeysFunc>(ClickHouseLibrary::LIBRARY_LOAD_KEYS_FUNC_NAME);
+    auto data_new_func = library->get<ClickHouseLibrary::LibraryDataNewFunc>(ClickHouseLibrary::LIBRARY_DATA_NEW_FUNC_NAME);
+    auto data_delete_func = library->get<ClickHouseLibrary::LibraryDataDeleteFunc>(ClickHouseLibrary::LIBRARY_DATA_DELETE_FUNC_NAME);
+
+    ClickHouseLibrary::LibraryData data_ptr = data_new_func(lib_data);
+    SCOPE_EXIT(data_delete_func(lib_data, data_ptr));
+
+    ClickHouseLibrary::RawClickHouseLibraryTable data = load_keys_func(data_ptr, &settings->strings, &request_cols);
     auto block = dataToBlock(description.sample_block, data);
-    SCOPE_EXIT(library->get<void (*)(decltype(lib_data), decltype(data_ptr))>("ClickHouseDictionary_v3_dataDelete")(lib_data, data_ptr));
+
     return std::make_shared<OneBlockInputStream>(block);
 }
 
 bool LibraryDictionarySource::isModified() const
 {
-    if (auto func_is_modified
-        = library->tryGet<bool (*)(decltype(lib_data), decltype(&settings->strings))>("ClickHouseDictionary_v3_isModified"))
+    if (auto func_is_modified = library->tryGet<ClickHouseLibrary::LibraryIsModifiedFunc>(
+        ClickHouseLibrary::LIBRARY_IS_MODIFIED_FUNC_NAME))
         return func_is_modified(lib_data, &settings->strings);
+
     return true;
 }
 
 bool LibraryDictionarySource::supportsSelectiveLoad() const
 {
-    if (auto func_supports_selective_load
-        = library->tryGet<bool (*)(decltype(lib_data), decltype(&settings->strings))>("ClickHouseDictionary_v3_supportsSelectiveLoad"))
+    if (auto func_supports_selective_load = library->tryGet<ClickHouseLibrary::LibrarySupportsSelectiveLoadFunc>(
+            ClickHouseLibrary::LIBRARY_SUPPORTS_SELECTIVE_LOAD_FUNC_NAME))
         return func_supports_selective_load(lib_data, &settings->strings);
+
     return true;
 }
 

src/Dictionaries/LibraryDictionarySourceExternal.cpp

@@ -6,10 +6,25 @@ namespace
 const char DICT_LOGGER_NAME[] = "LibraryDictionarySourceExternal";
 }
 
-void ClickHouseLibrary::log(ClickHouseLibrary::LogLevel level, ClickHouseLibrary::CString msg)
+namespace ClickHouseLibrary
 {
-    using ClickHouseLibrary::LogLevel;
 
+std::string_view LIBRARY_CREATE_NEW_FUNC_NAME = "ClickHouseDictionary_v3_libNew";
+std::string_view LIBRARY_CLONE_FUNC_NAME = "ClickHouseDictionary_v3_libClone";
+std::string_view LIBRARY_DELETE_FUNC_NAME = "ClickHouseDictionary_v3_libDelete";
+
+std::string_view LIBRARY_DATA_NEW_FUNC_NAME = "ClickHouseDictionary_v3_dataNew";
+std::string_view LIBRARY_DATA_DELETE_FUNC_NAME = "ClickHouseDictionary_v3_dataDelete";
+
+std::string_view LIBRARY_LOAD_ALL_FUNC_NAME = "ClickHouseDictionary_v3_loadAll";
+std::string_view LIBRARY_LOAD_IDS_FUNC_NAME = "ClickHouseDictionary_v3_loadIds";
+std::string_view LIBRARY_LOAD_KEYS_FUNC_NAME = "ClickHouseDictionary_v3_loadKeys";
+
+std::string_view LIBRARY_IS_MODIFIED_FUNC_NAME = "ClickHouseDictionary_v3_isModified";
+std::string_view LIBRARY_SUPPORTS_SELECTIVE_LOAD_FUNC_NAME = "ClickHouseDictionary_v3_supportsSelectiveLoad";
+
+void log(LogLevel level, CString msg)
+{
     auto & logger = Poco::Logger::get(DICT_LOGGER_NAME);
     switch (level)
     {
@@ -47,3 +62,5 @@ void ClickHouseLibrary::log(ClickHouseLibrary::LogLevel level, ClickHouseLibrary
             break;
     }
 }
+
+}

src/Dictionaries/LibraryDictionarySourceExternal.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <cstdint>
+#include <string>
 
 #define CLICKHOUSE_DICTIONARY_LIBRARY_API 1
 
@@ -61,4 +62,49 @@ enum LogLevel
 };
 
 void log(LogLevel level, CString msg);
+
+extern std::string_view LIBRARY_CREATE_NEW_FUNC_NAME;
+extern std::string_view LIBRARY_CLONE_FUNC_NAME;
+extern std::string_view LIBRARY_DELETE_FUNC_NAME;
+
+extern std::string_view LIBRARY_DATA_NEW_FUNC_NAME;
+extern std::string_view LIBRARY_DATA_DELETE_FUNC_NAME;
+
+extern std::string_view LIBRARY_LOAD_ALL_FUNC_NAME;
+extern std::string_view LIBRARY_LOAD_IDS_FUNC_NAME;
+extern std::string_view LIBRARY_LOAD_KEYS_FUNC_NAME;
+
+extern std::string_view LIBRARY_IS_MODIFIED_FUNC_NAME;
+extern std::string_view LIBRARY_SUPPORTS_SELECTIVE_LOAD_FUNC_NAME;
+
+using LibraryContext = void *;
+
+using LibraryLoggerFunc = void (*)(LogLevel, CString /* message */);
+
+using LibrarySettings = CStrings *;
+
+using LibraryNewFunc = LibraryContext (*)(LibrarySettings, LibraryLoggerFunc);
+using LibraryCloneFunc = LibraryContext (*)(LibraryContext);
+using LibraryDeleteFunc = void (*)(LibraryContext);
+
+using LibraryData = void *;
+using LibraryDataNewFunc = LibraryData (*)(LibraryContext);
+using LibraryDataDeleteFunc = void (*)(LibraryContext, LibraryData);
+
+/// Can be safely casted into const Table * with static_cast<const ClickHouseLibrary::Table *>
+using RawClickHouseLibraryTable = void *;
+using RequestedColumnsNames = CStrings *;
+
+using LibraryLoadAllFunc = RawClickHouseLibraryTable (*)(LibraryData, LibrarySettings, RequestedColumnsNames);
+
+using RequestedIds = const VectorUInt64 *;
+using LibraryLoadIdsFunc = RawClickHouseLibraryTable (*)(LibraryData, LibrarySettings, RequestedColumnsNames, RequestedIds);
+
+using RequestedKeys = Table *;
+/// There is no requested columns names for load keys func
+using LibraryLoadKeysFunc = RawClickHouseLibraryTable (*)(LibraryData, LibrarySettings, RequestedKeys);
+
+using LibraryIsModifiedFunc = bool (*)(LibraryContext, LibrarySettings);
+using LibrarySupportsSelectiveLoadFunc = bool (*)(LibraryContext, LibrarySettings);
+
 }