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.

 - 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.

.mci.yml

@@ -120,6 +120,8 @@ tasks:
                   make examples
                   ${test_params} ./src/bsoncxx/test/test_bson
                   ${test_params} ./src/mongocxx/test/test_driver
+                  ${test_params} ./src/mongocxx/test/test_instance
+
                   # The break -1 is a hack to cause us to have a bad
                   # exit status if any test exits with a bad exit
                   # status.

.travis.yml

@@ -80,6 +80,9 @@ script:
     # Run mongocxx tests with catch
     - ./src/mongocxx/test/test_driver
 
+    # Run mongocxx instance tests with catch
+    - ./src/mongocxx/test/test_instance
+
     # Install headers and libs for the examples
     - make install
 

src/mongocxx/collection.cpp

@@ -704,10 +704,12 @@ bsoncxx::document::value collection::create_index(view_or_value keys,
         return bsoncxx::builder::stream::document{} << "name" << *options.name()
                                                     << bsoncxx::builder::stream::finalize;
     } else {
-        return bsoncxx::builder::stream::document{}
-               << "name"
-               << std::string{libmongoc::collection_keys_to_index_string(bson_keys.bson())}
-               << bsoncxx::builder::stream::finalize;
+        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" << keys
+                                                    << bsoncxx::builder::stream::finalize;
     }
 }
 
@@ -726,6 +728,8 @@ 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/exception/error_code.hpp

@@ -22,7 +22,8 @@ namespace mongocxx {
 MONGOCXX_INLINE_NAMESPACE_BEGIN
 
 enum class error_code : std::int32_t {
-    k_invalid_client_object = 1,
+    k_instance_already_exists = 1,
+    k_invalid_client_object,
     k_invalid_collection_object,
     k_invalid_database_object,
     k_invalid_parameter,

src/mongocxx/exception/private/error_category.cpp

@@ -401,6 +401,8 @@ class mongocxx_error_category_impl final : public std::error_category {
                 return "invalid attempt to set an unknown read concern level";
             case error_code::k_unknown_write_concern:
                 return "invalid attempt to set an unknown write concern level";
+            case error_code::k_instance_already_exists:
+                return "cannot create more than one mongocxx::instance object";
             default:
                 return "unknown mongocxx error";
         }

src/mongocxx/instance.cpp

@@ -14,10 +14,13 @@
 
 #include <mongocxx/instance.hpp>
 
+#include <mutex>
 #include <utility>
 
 #include <bsoncxx/stdx/make_unique.hpp>
+#include <mongocxx/exception/logic_error.hpp>
 #include <mongocxx/logger.hpp>
+#include <mongocxx/exception/private/error_code.hpp>
 #include <mongocxx/private/libmongoc.hpp>
 
 #include <mongocxx/config/private/prelude.hpp>
@@ -57,6 +60,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 {
@@ -85,13 +92,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) {
+        throw logic_error(make_error_code(error_code::k_instance_already_exists));
+    }
+    _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/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 (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
     target_compile_options(test_driver PRIVATE /bigobj)
 endif()
 
 add_test(driver test_driver)
+add_test(instance test_instance)

src/mongocxx/test/bulk_write.cpp

@@ -17,14 +17,16 @@
 
 #include <bsoncxx/builder/stream/document.hpp>
 #include <bsoncxx/types.hpp>
-
+#include <mongocxx/instance.hpp>
 #include <mongocxx/private/libmongoc.hpp>
 #include <mongocxx/bulk_write.hpp>
 #include <mongocxx/write_concern.hpp>
 
 using namespace mongocxx;
 
 TEST_CASE("a bulk_write will setup a mongoc bulk operation", "[bulk_write]") {
+    instance::current();
+
     auto construct = libmongoc::bulk_operation_new.create_instance();
     bool construct_called = false;
     bool ordered_value = false;
@@ -50,6 +52,8 @@ TEST_CASE("a bulk_write will setup a mongoc bulk operation", "[bulk_write]") {
 }
 
 TEST_CASE("destruction of a bulk_write will destroy mongoc operation", "[bulk_write]") {
+    instance::current();
+
     auto destruct = libmongoc::bulk_operation_destroy.create_instance();
     bool destruct_called = false;
 
@@ -104,6 +108,8 @@ class FilteredDocumentFun : public SingleDocumentFun {
 };
 
 TEST_CASE("passing write operations to append calls corresponding C function", "[bulk_write]") {
+    instance::current();
+
     bulk_write bw;
     bsoncxx::builder::stream::document filter_builder, doc_builder, update_doc_builder;
     filter_builder << "_id" << 1;

src/mongocxx/test/client.cpp

@@ -17,6 +17,7 @@
 
 #include <mongocxx/client.hpp>
 #include <mongocxx/exception/logic_error.hpp>
+#include <mongocxx/instance.hpp>
 #include <mongocxx/private/libmongoc.hpp>
 #include <mongocxx/uri.hpp>
 
@@ -25,24 +26,33 @@ using namespace mongocxx;
 TEST_CASE("A default constructed client is false-ish", "[client]") {
     MOCK_CLIENT
 
+    instance::current();
+
     client a;
     REQUIRE(!a);
 }
 
 TEST_CASE("A default constructed client cannot perform operations", "[client]") {
+    instance::current();
+
     client a;
     REQUIRE_THROWS_AS(a.list_databases(), mongocxx::logic_error);
 }
 
 TEST_CASE("A client constructed with a URI is truthy", "[client]") {
     MOCK_CLIENT
 
+    instance::current();
+
     client a{uri{}};
     REQUIRE(a);
 }
 
 TEST_CASE("A client connects to a provided mongodb uri", "[client]") {
     MOCK_CLIENT
+
+    instance::current();
+
     std::string expected_url("mongodb://mongodb.example.com:9999");
     uri mongodb_uri(expected_url);
     std::string actual_url{};
@@ -62,6 +72,9 @@ TEST_CASE("A client connects to a provided mongodb uri", "[client]") {
 
 TEST_CASE("A client cleans up its underlying mongoc client on destruction", "[client]") {
     MOCK_CLIENT
+
+    instance::current();
+
     bool destroy_called = false;
     client_destroy->interpose([&](mongoc_client_t*) { destroy_called = true; });
 
@@ -76,6 +89,8 @@ TEST_CASE("A client cleans up its underlying mongoc client on destruction", "[cl
 TEST_CASE("A client supports move operations", "[client]") {
     MOCK_CLIENT
 
+    instance::current();
+
     client a{uri{}};
 
     bool called = false;
@@ -94,6 +109,8 @@ TEST_CASE("A client supports move operations", "[client]") {
 TEST_CASE("A client has a settable Read Concern", "[collection]") {
     MOCK_CLIENT
 
+    instance::current();
+
     client mongo_client{uri{}};
 
     auto client_set_rc_called = false;
@@ -116,6 +133,8 @@ TEST_CASE("A client has a settable Read Concern", "[collection]") {
 TEST_CASE("A client's read preferences may be set and obtained", "[client]") {
     MOCK_CLIENT
 
+    instance::current();
+
     client mongo_client{uri{}};
     read_preference preference{read_preference::read_mode::k_secondary_preferred};
 
@@ -143,6 +162,8 @@ TEST_CASE("A client's read preferences may be set and obtained", "[client]") {
 TEST_CASE("A client's write concern may be set and obtained", "[client]") {
     MOCK_CLIENT
 
+    instance::current();
+
     client mongo_client{uri{}};
     write_concern concern;
     concern.majority(std::chrono::milliseconds(100));
@@ -181,6 +202,9 @@ TEST_CASE("A client's write concern may be set and obtained", "[client]") {
 
 TEST_CASE("A client can create a named database object", "[client]") {
     MOCK_CLIENT
+
+    instance::current();
+
     auto database_get = libmongoc::client_get_database.create_instance();
     database_get->interpose([](mongoc_client_t*, const char*) { return nullptr; }).forever();
     auto database_destroy = libmongoc::database_destroy.create_instance();

src/mongocxx/test/collection.cpp

@@ -21,10 +21,10 @@
 #include <bsoncxx/json.hpp>
 #include <bsoncxx/stdx/string_view.hpp>
 #include <bsoncxx/types.hpp>
-
 #include <mongocxx/collection.hpp>
 #include <mongocxx/client.hpp>
 #include <mongocxx/exception/logic_error.hpp>
+#include <mongocxx/instance.hpp>
 #include <mongocxx/insert_many_builder.hpp>
 #include <mongocxx/pipeline.hpp>
 #include <mongocxx/read_concern.hpp>
@@ -33,11 +33,15 @@ using namespace bsoncxx::builder::stream;
 using namespace mongocxx;
 
 TEST_CASE("A default constructed collection cannot perform operations", "[collection]") {
+    instance::current();
+
     collection c;
     REQUIRE_THROWS_AS(c.name(), mongocxx::logic_error);
 }
 
 TEST_CASE("collection copy", "[collection]") {
+    instance::current();
+
     client mongodb_client{uri{}};
     database db = mongodb_client["test"];
 
@@ -54,6 +58,8 @@ TEST_CASE("collection copy", "[collection]") {
 }
 
 TEST_CASE("collection renaming", "[collection]") {
+    instance::current();
+
     client mongodb_client{uri{}};
     database db = mongodb_client["test"];
 
@@ -70,6 +76,8 @@ TEST_CASE("collection renaming", "[collection]") {
 }
 
 TEST_CASE("CRUD functionality", "[driver::collection]") {
+    instance::current();
+
     client mongodb_client{uri{}};
     database db = mongodb_client["test"];
     collection coll = db["mongo_cxx_driver"];
@@ -610,6 +618,8 @@ TEST_CASE("read_concern is inherited from parent", "[collection]") {
 }
 
 TEST_CASE("create_index returns index name", "[collection]") {
+    instance::current();
+
     client mongodb_client{uri{}};
     database db = mongodb_client["test"];
     collection coll = db["collection"];