CXX-813 Fix memory errors exposed by ASAN and Valgrind

- We can't assume that libmongoc has automatically called init/cleanup
  for us. It only does that on some platforms. That makes it mandatory
  to have an instance object. Many of our tests didn't do that, and it
  would be painful for users. Set things up so that an instance is
  implicitly created as needed, and provide ways to retrieve the
  instance. This required moving the tests for instance to its own
  process, among other nuisances.

- The collection::create_index method was not cleaning up the keys that
  had been allocated by libbson. Add the needed bson_free call.

- The collection::distinct method was not cleaning up the temorary
  database object that it constructs. Add the needed database_destroy
  call.

- Fix some mocks so that they write to the bson_error_t out parameter
  when returning a non-successful code, as otherwise these lead to
  read-from-uninit errors when we promote the uninitialized bson_error_t
  to an exception.

Author: acmorrow

.mci.yml

@@ -116,6 +116,7 @@ tasks:
                   make
                   ${test_params} ./src/bsoncxx/test/test_bson
                   ${test_params} ./src/mongocxx/test/test_driver
+                  ${test_params} ./src/mongocxx/test/test_instance
                   make run-examples
         - func: "stop_mongod"
 

.travis.yml

@@ -78,5 +78,8 @@ script:
     # Run mongocxx tests with catch
     - ./src/mongocxx/test/test_driver
 
+        # Run mongocxx instance tests with catch
+    - ./src/mongocxx/test/test_instance
+
     # Run all examples
     - make run-examples

src/mongocxx/client.cpp

@@ -23,6 +23,7 @@
 #include <mongocxx/exception/private/error_category.hpp>
 #include <mongocxx/exception/private/error_code.hpp>
 #include <mongocxx/exception/private/mongoc_error.hpp>
+#include <mongocxx/instance.hpp>
 #include <mongocxx/private/client.hpp>
 #include <mongocxx/private/read_concern.hpp>
 #include <mongocxx/private/read_preference.hpp>
@@ -36,7 +37,8 @@ MONGOCXX_INLINE_NAMESPACE_BEGIN
 client::client() noexcept = default;
 
 client::client(const class uri& uri, const options::client& options)
-    : _impl(stdx::make_unique<impl>(libmongoc::client_new_from_uri(uri._impl->uri_t))) {
+    : _instance(&instance::current()),
+      _impl(stdx::make_unique<impl>(libmongoc::client_new_from_uri(uri._impl->uri_t))) {
     if (options.ssl_opts()) {
 #if defined(MONGOC_HAVE_SSL)
         auto mongoc_opts = options::make_ssl_opts(*options.ssl_opts());
@@ -47,8 +49,9 @@ client::client(const class uri& uri, const options::client& options)
     }
 }
 
-client::client(void* implementation)
-    : _impl{stdx::make_unique<impl>(static_cast<::mongoc_client_t*>(implementation))} {
+client::client(class instance* instance, void* implementation)
+    : _instance(instance),
+      _impl{stdx::make_unique<impl>(static_cast<::mongoc_client_t*>(implementation))} {
 }
 
 client::client(client&&) noexcept = default;

src/mongocxx/client.hpp

@@ -30,6 +30,8 @@
 namespace mongocxx {
 MONGOCXX_INLINE_NAMESPACE_BEGIN
 
+class instance;
+
 ///
 /// Class representing a client connection to MongoDB.
 ///
@@ -80,6 +82,11 @@ class MONGOCXX_API client {
     ///
     ~client();
 
+    ///
+    /// Obtain the instance associated with this client
+    ///
+    MONGOCXX_INLINE class instance& instance() const noexcept;
+
     ///
     /// Returns true if the client is valid, meaning it was not default constructed
     /// or moved from.
@@ -202,16 +209,21 @@ class MONGOCXX_API client {
     friend class database;
     friend class pool;
 
-    MONGOCXX_PRIVATE explicit client(void* implementation);
+    MONGOCXX_PRIVATE explicit client(class instance*, void* implementation);
 
     class MONGOCXX_PRIVATE impl;
 
     MONGOCXX_PRIVATE impl& _get_impl();
     MONGOCXX_PRIVATE const impl& _get_impl() const;
 
+    class instance* _instance;
     std::unique_ptr<impl> _impl;
 };
 
+MONGOCXX_INLINE instance& client::instance() const noexcept {
+    return *_instance;
+}
+
 MONGOCXX_INLINE database client::operator[](bsoncxx::string::view_or_value name) const & {
     return database(name);
 }

src/mongocxx/collection.cpp

@@ -702,9 +702,16 @@ bsoncxx::document::value collection::create_index(view_or_value keys,
         return bsoncxx::builder::stream::document{} << "name" << *options.name()
                                                     << bsoncxx::builder::stream::finalize;
     } else {
+
+        const auto keys = libmongoc::collection_keys_to_index_string(bson_keys.bson());
+
+        const auto clean_keys = make_guard([&]{
+                bson_free(keys);
+        });
+
         return bsoncxx::builder::stream::document{}
                << "name"
-               << std::string{libmongoc::collection_keys_to_index_string(bson_keys.bson())}
+               << keys
                << bsoncxx::builder::stream::finalize;
     }
 }
@@ -724,6 +731,10 @@ cursor collection::distinct(bsoncxx::string::view_or_value field_name, view_or_v
     auto database = libmongoc::client_get_database(_get_impl().client_impl->client_t,
                                                    _get_impl().database_name.c_str());
 
+    const auto cleanup_database = make_guard([&] {
+            libmongoc::database_destroy(database);
+    });
+
     auto result = libmongoc::database_command(database, MONGOC_QUERY_NONE, 0, 0, 0,
                                               command_bson.bson(), NULL, NULL);
 

src/mongocxx/instance.cpp

@@ -14,6 +14,7 @@
 
 #include <mongocxx/instance.hpp>
 
+#include <mutex>
 #include <utility>
 
 #include <bsoncxx/stdx/make_unique.hpp>
@@ -56,6 +57,10 @@ void user_log_handler(::mongoc_log_level_t mongoc_log_level, const char *log_dom
                                         stdx::string_view{log_domain}, stdx::string_view{message});
 }
 
+std::recursive_mutex instance_mutex;
+instance* current_instance = nullptr;
+std::unique_ptr<instance> global_instance;
+
 }  // namespace
 
 class instance::impl {
@@ -84,13 +89,33 @@ class instance::impl {
 instance::instance() : instance(nullptr) {
 }
 
-instance::instance(std::unique_ptr<logger> logger)
-    : _impl(stdx::make_unique<impl>(std::move(logger))) {
+instance::instance(std::unique_ptr<logger> logger) {
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex);
+    if (current_instance)
+        std::abort();
+    _impl = stdx::make_unique<impl>(std::move(logger));
+    current_instance = this;
 }
 
 instance::instance(instance &&) noexcept = default;
 instance &instance::operator=(instance &&) noexcept = default;
-instance::~instance() = default;
+
+instance::~instance() {
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex);
+    if (current_instance != this)
+        std::abort();
+    _impl.reset();
+    current_instance = nullptr;
+}
+
+instance& instance::current() {
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex);
+    if (!current_instance) {
+        global_instance.reset(new instance);
+        current_instance = global_instance.get();
+    }
+    return *current_instance;
+}
 
 MONGOCXX_INLINE_NAMESPACE_END
 }  // namespace mongocxx

src/mongocxx/instance.hpp

@@ -26,7 +26,7 @@ class logger;
 ///
 /// Class representing an instance of the MongoDB driver.
 ///
-/// Life cycle: An instance of the driver *MUST* be kept around.
+/// Life cycle: A unique instance of the driver *MUST* be kept around.
 ///
 class MONGOCXX_API instance {
    public:
@@ -56,6 +56,14 @@ class MONGOCXX_API instance {
     ///
     ~instance();
 
+    ///
+    /// Returns the current unique instance of the driver. If an
+    /// instance was explicitly created, that will be returned. If no
+    /// instance has yet been created, a default instance will be
+    /// constructed and returned.
+    ///
+    static instance& current();
+
    private:
     class MONGOCXX_PRIVATE impl;
     std::unique_ptr<impl> _impl;

src/mongocxx/pool.cpp

@@ -24,6 +24,7 @@
 #include <mongocxx/exception/error_code.hpp>
 #include <mongocxx/exception/exception.hpp>
 #include <mongocxx/exception/private/error_code.hpp>
+#include <mongocxx/instance.hpp>
 #include <mongocxx/private/client.hpp>
 #include <mongocxx/private/pool.hpp>
 #include <mongocxx/private/ssl.hpp>
@@ -42,7 +43,8 @@ void pool::_release(client* client) {
 pool::~pool() = default;
 
 pool::pool(const uri& mongodb_uri, stdx::optional<options::ssl> ssl_options)
-    : _impl{stdx::make_unique<impl>(libmongoc::client_pool_new(mongodb_uri._impl->uri_t),
+    : _instance(&instance::current()),
+      _impl{stdx::make_unique<impl>(libmongoc::client_pool_new(mongodb_uri._impl->uri_t),
                                     std::move(ssl_options))} {
     if (_impl->ssl_options) {
 #if defined(MONGOC_HAVE_SSL)
@@ -58,15 +60,15 @@ pool::pool(const uri& mongodb_uri, stdx::optional<options::ssl> ssl_options)
 }
 
 pool::entry pool::acquire() {
-    return entry(new client(libmongoc::client_pool_pop(_impl->client_pool_t)),
+    return entry(new client(&instance(), libmongoc::client_pool_pop(_impl->client_pool_t)),
                  [this](client* client) { _release(client); });
 }
 
 stdx::optional<pool::entry> pool::try_acquire() {
     auto cli = libmongoc::client_pool_try_pop(_impl->client_pool_t);
     if (!cli) return stdx::nullopt;
 
-    return pool::entry{new client(cli), [this](client* client) { _release(client); }};
+    return pool::entry{new client(&instance(), cli), [this](client* client) { _release(client); }};
 }
 
 MONGOCXX_INLINE_NAMESPACE_END

src/mongocxx/pool.hpp

@@ -29,6 +29,7 @@ namespace mongocxx {
 MONGOCXX_INLINE_NAMESPACE_BEGIN
 
 class client;
+class instance;
 
 ///
 /// A pool of @c client objects associated with a MongoDB deployment.
@@ -64,6 +65,11 @@ class MONGOCXX_API pool {
     ///
     ~pool();
 
+    ///
+    /// Obtain the driver instance associated with this pool
+    ///
+    MONGOCXX_INLINE class instance& instance() const noexcept;
+
     ///
     /// An entry is a handle on a @c client object acquired via the pool.
     ///
@@ -88,9 +94,15 @@ class MONGOCXX_API pool {
     MONGOCXX_PRIVATE void _release(client* client);
 
     class MONGOCXX_PRIVATE impl;
+
+    class instance* _instance;
     const std::unique_ptr<impl> _impl;
 };
 
+MONGOCXX_INLINE instance& pool::instance() const noexcept {
+    return *_instance;
+}
+
 MONGOCXX_INLINE_NAMESPACE_END
 }  // namespace mongocxx
 

src/mongocxx/test/CMakeLists.txt

@@ -12,6 +12,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+include_directories(
+    ${THIRD_PARTY_SOURCE_DIR}/catch/include
+)
+
+link_directories(${LIBMONGOC_LIBRARY_DIRS} ${LIBBSON_LIBRARY_DIRS})
+
 set(mongocxx_test_sources
     CMakeLists.txt
     bulk_write.cpp
@@ -20,7 +26,6 @@ set(mongocxx_test_sources
     collection_mocked.cpp
     database.cpp
     hint.cpp
-    instance.cpp
     model/update_many.cpp
     options/aggregate.cpp
     options/create_collection.cpp
@@ -45,21 +50,23 @@ set(mongocxx_test_sources
     write_concern.cpp
 )
 
-include_directories(
-    ${THIRD_PARTY_SOURCE_DIR}/catch/include
-)
-
-link_directories(${LIBMONGOC_LIBRARY_DIRS} ${LIBBSON_LIBRARY_DIRS})
-
 add_executable(test_driver
     ${THIRD_PARTY_SOURCE_DIR}/catch/main.cpp
     ${mongocxx_test_sources}
 )
 
 target_link_libraries(test_driver mongocxx_mocked)
 
+add_executable(test_instance
+  ${THIRD_PARTY_SOURCE_DIR}/catch/main.cpp
+  instance.cpp
+)
+
+target_link_libraries(test_instance mongocxx_mocked)
+
 if (WIN32)
     target_compile_options(test_driver PRIVATE /bigobj)
 endif()
 
 add_test(driver test_driver)
+add_test(instance test_instance)