SWIFT-294, SWIFT-394 Fix memory leaks (#237)

Author: kmahar

Sources/MongoSwift/APM.swift

@@ -48,8 +48,8 @@ public struct CommandStartedEvent: MongoEvent, InitializableFromOpaquePointer {
 
     /// Initializes a CommandStartedEvent from an OpaquePointer to a mongoc_apm_command_started_t
     fileprivate init(_ event: OpaquePointer) {
-        // swiftlint:disable:next force_unwrapping - documented as always returning a value.
-        self.command = Document(fromPointer: mongoc_apm_command_started_get_command(event)!)
+        // we have to copy because libmongoc owns the pointer.
+        self.command = Document(copying: mongoc_apm_command_started_get_command(event))
         self.databaseName = String(cString: mongoc_apm_command_started_get_database_name(event))
         self.commandName = String(cString: mongoc_apm_command_started_get_command_name(event))
         self.requestId = mongoc_apm_command_started_get_request_id(event)
@@ -89,8 +89,8 @@ public struct CommandSucceededEvent: MongoEvent, InitializableFromOpaquePointer
     /// Initializes a CommandSucceededEvent from an OpaquePointer to a mongoc_apm_command_succeeded_t
     fileprivate init(_ event: OpaquePointer) {
         self.duration = mongoc_apm_command_succeeded_get_duration(event)
-        // swiftlint:disable:next force_unwrapping - documented as always returning a value.
-        self.reply = Document(fromPointer: mongoc_apm_command_succeeded_get_reply(event)!)
+        // we have to copy because libmongoc owns the pointer.
+        self.reply = Document(copying: mongoc_apm_command_succeeded_get_reply(event))
         self.commandName = String(cString: mongoc_apm_command_succeeded_get_command_name(event))
         self.requestId = mongoc_apm_command_succeeded_get_request_id(event)
         self.operationId = mongoc_apm_command_succeeded_get_operation_id(event)
@@ -319,8 +319,8 @@ public struct ServerHeartbeatSucceededEvent: MongoEvent, InitializableFromOpaque
     /// Initializes a ServerHeartbeatSucceededEvent from an OpaquePointer to a mongoc_apm_server_heartbeat_succeeded_t
     fileprivate init(_ event: OpaquePointer) {
         self.duration = mongoc_apm_server_heartbeat_succeeded_get_duration(event)
-        // swiftlint:disable:next force_unwrapping - documented as always returning a value.
-        self.reply = Document(fromPointer: mongoc_apm_server_heartbeat_succeeded_get_reply(event)!)
+        // we have to copy because libmongoc owns the pointer.
+        self.reply = Document(copying: mongoc_apm_server_heartbeat_succeeded_get_reply(event))
         self.connectionId = ConnectionId(mongoc_apm_server_heartbeat_succeeded_get_host(event))
     }
 }

Sources/MongoSwift/BSON/BSONValue.swift

@@ -127,7 +127,7 @@ extension Array: BSONValue {
                 throw RuntimeError.internalError(message: "Failed to create an Array from iterator")
             }
 
-            let arrDoc = Document(fromPointer: arrayData)
+            let arrDoc = Document(stealing: arrayData)
 
             guard let arr = arrDoc.values as? Array else {
                 fatalError("Failed to cast values for document \(arrDoc) to array")
@@ -658,7 +658,7 @@ public struct CodeWithScope: BSONValue, Equatable, Codable {
             guard let scopeData = bson_new_from_data(scopePointer.pointee, Int(scopeLength)) else {
                 throw RuntimeError.internalError(message: "Failed to create a bson_t from scope data")
             }
-            let scopeDoc = Document(fromPointer: scopeData)
+            let scopeDoc = Document(stealing: scopeData)
 
             return self.init(code: code, scope: scopeDoc)
         }

Sources/MongoSwift/BSON/Document.swift

@@ -20,14 +20,22 @@ public class DocumentStorage {
         return Int(bson_count_keys(self.pointer))
     }
 
+    /// Initializes a new, empty `DocumentStorage`.
     internal init() {
         self.pointer = bson_new()
     }
 
-    internal init(fromPointer pointer: BSONPointer) {
+    /// Initializes a new `DocumentStorage` by copying over the data from the provided `bson_t`.
+    internal init(copying pointer: BSONPointer) {
         self.pointer = bson_copy(pointer)
     }
 
+    /// Initializes a new `DocumentStorage` that uses the provided `bson_t` as its backing storage.
+    /// The newly created instance will handle cleaning up the pointer upon deinitialization.
+    internal init(stealing pointer: MutableBSONPointer) {
+        self.pointer = pointer
+    }
+
     /// Cleans up internal state.
     deinit {
         guard let pointer = self.pointer else {
@@ -57,17 +65,30 @@ extension Document {
     }
 
     /**
-     * Initializes a `Document` from a pointer to a bson_t. Uses a copy
-     * of `bsonData`, so the caller is responsible for freeing the original
-     * memory.
+     * Initializes a `Document` from a pointer to a `bson_t` by making a copy of the data. The `bson_t`'s owner is
+     * responsible for freeing the original.
+     *
+     * - Parameters:
+     *   - pointer: a BSONPointer
+     *
+     * - Returns: a new `Document`
+     */
+    internal init(copying pointer: BSONPointer) {
+        self.storage = DocumentStorage(copying: pointer)
+        self.count = self.storage.count
+    }
+
+    /**
+     * Initializes a `Document` from a pointer to a `bson_t`, "stealing" the `bson_t` to use for underlying storage/
+     * The `bson_t` must not be modified or freed by others after it is used here/
      *
      * - Parameters:
-     *   - fromPointer: a BSONPointer
+     *   - pointer: a MutableBSONPointer
      *
      * - Returns: a new `Document`
      */
-    internal init(fromPointer pointer: BSONPointer) {
-        self.storage = DocumentStorage(fromPointer: pointer)
+    internal init(stealing pointer: MutableBSONPointer) {
+        self.storage = DocumentStorage(stealing: pointer)
         self.count = self.storage.count
     }
 
@@ -232,7 +253,7 @@ extension Document {
      */
     private mutating func copyStorageIfRequired() {
         if !isKnownUniquelyReferenced(&self.storage) {
-            self.storage = DocumentStorage(fromPointer: self.data)
+            self.storage = DocumentStorage(copying: self.data)
         }
     }
 
@@ -319,7 +340,7 @@ extension Document {
      *   - A `UserError.invalidArgumentError` if the data passed in is invalid JSON.
      */
     public init(fromJSON: Data) throws {
-        self.storage = DocumentStorage(fromPointer: try fromJSON.withUnsafeBytes { (bytes: UnsafePointer<UInt8>) in
+        self.storage = DocumentStorage(stealing: try fromJSON.withUnsafeBytes { (bytes: UnsafePointer<UInt8>) in
             var error = bson_error_t()
             guard let bson = bson_new_from_json(bytes, fromJSON.count, &error) else {
                 if error.domain == BSON_ERROR_JSON {
@@ -328,11 +349,7 @@ extension Document {
                 throw RuntimeError.internalError(message: toErrorString(error))
             }
 
-#if compiler(>=5.0)
             return bson
-#else
-            return UnsafePointer(bson)
-#endif
         })
         self.count = self.storage.count
     }
@@ -348,7 +365,7 @@ extension Document {
 
     /// Constructs a `Document` from raw BSON `Data`.
     public init(fromBSON: Data) {
-        self.storage = DocumentStorage(fromPointer: fromBSON.withUnsafeBytes { (bytes: UnsafePointer<UInt8>) in
+        self.storage = DocumentStorage(stealing: fromBSON.withUnsafeBytes { (bytes: UnsafePointer<UInt8>) in
             bson_new_from_data(bytes, fromBSON.count)
         })
         self.count = self.storage.count
@@ -456,7 +473,7 @@ extension Document: BSONValue {
                 throw RuntimeError.internalError(message: "Failed to create a Document from iterator")
             }
 
-            return self.init(fromPointer: docData)
+            return self.init(stealing: docData)
         }
     }
 }

Sources/MongoSwift/MongoClient.swift

@@ -168,6 +168,7 @@ public class MongoClient {
         guard let uri = mongoc_uri_new_with_error(connectionString, &error) else {
             throw parseMongocError(error)
         }
+        defer { mongoc_uri_destroy(uri) }
 
         // if retryWrites is specified, set it on the uri (libmongoc does not provide api for setting it on the client).
         if let rw = options?.retryWrites {

Sources/MongoSwift/MongoCursor.swift

@@ -80,7 +80,10 @@ public class MongoCursor<T: Codable>: Sequence, IteratorProtocol {
         }
 
         var replyPtr = UnsafeMutablePointer<BSONPointer?>.allocate(capacity: 1)
-        defer { replyPtr.deallocate() }
+        defer {
+            replyPtr.deinitialize(count: 1)
+            replyPtr.deallocate()
+        }
 
         var error = bson_error_t()
         guard mongoc_cursor_error_document(self._cursor, &error, replyPtr) else {
@@ -90,7 +93,8 @@ public class MongoCursor<T: Codable>: Sequence, IteratorProtocol {
         // If a reply is present, it implies the error occurred on the server. This *should* always be a commandError,
         // but we will still parse the mongoc error to cover all cases.
         if let docPtr = replyPtr.pointee {
-            let reply = Document(fromPointer: docPtr)
+            // we have to copy because libmongoc owns the pointer.
+            let reply = Document(copying: docPtr)
             return parseMongocError(error, errorLabels: reply["errorLabels"] as? [String])
         }
 
@@ -115,9 +119,13 @@ public class MongoCursor<T: Codable>: Sequence, IteratorProtocol {
         guard mongoc_cursor_next(self._cursor, out) else {
             return nil
         }
-        // swiftlint:disable:next force_unwrapping - if mongoc_cursor_next returned `true`, this is filled out.
-        let doc = Document(fromPointer: out.pointee!)
 
+        guard let pointee = out.pointee else {
+            fatalError("mongoc_cursor_next returned true, but document is nil")
+        }
+
+        // we have to copy because libmongoc owns the pointer.
+        let doc = Document(copying: pointee)
         do {
             let outDoc = try self.decoder.decode(T.self, from: doc)
             self.swiftError = nil

Sources/MongoSwift/ReadPreference.swift

@@ -70,8 +70,8 @@ public final class ReadPreference {
         guard let bson = mongoc_read_prefs_get_tags(self._readPreference) else {
             fatalError("Failed to retrieve read preference tags")
         }
-
-        let wrapped = Document(fromPointer: bson)
+        // we have to copy because libmongoc owns the pointer.
+        let wrapped = Document(copying: bson)
 
         // swiftlint:disable:next force_cast
         return wrapped.values as! [Document]

Sources/MongoSwift/SDAM.swift

@@ -171,8 +171,8 @@ public struct ServerDescription {
     internal init(_ description: OpaquePointer) {
         self.connectionId = ConnectionId(mongoc_server_description_host(description))
         self.roundTripTime = mongoc_server_description_round_trip_time(description)
-        // swiftlint:disable:next force_unwrapping - documented as always returning a value.
-        let isMaster = Document(fromPointer: mongoc_server_description_ismaster(description)!)
+        // we have to copy because libmongoc owns the pointer.
+        let isMaster = Document(copying: mongoc_server_description_ismaster(description))
         self.parseIsMaster(isMaster)
 
         let serverType = String(cString: mongoc_server_description_type(description))
@@ -250,6 +250,8 @@ public struct TopologyDescription {
 
         var size = size_t()
         let serverData = mongoc_topology_description_get_servers(description, &size)
+        defer { mongoc_server_descriptions_destroy_all(serverData, size) }
+
         let buffer = UnsafeBufferPointer(start: serverData, count: size)
         if size > 0 {
             // swiftlint:disable:next force_unwrapping - documented as always returning a value.

Tests/MongoSwiftTests/DocumentTests.swift

@@ -481,7 +481,7 @@ final class DocumentTests: MongoSwiftTestCase {
     // test replacing `Overwritable` types with values of their own type
     func testOverwritable() throws {
         // make a deep copy so we start off with uniquely referenced storage
-        var doc = Document(fromPointer: DocumentTests.overwritables.data)
+        var doc = Document(copying: DocumentTests.overwritables.data)
 
         // save a reference to original bson_t so we can verify it doesn't change
         let pointer = doc.data
@@ -555,7 +555,7 @@ final class DocumentTests: MongoSwiftTestCase {
     // test replacing some of the non-Overwritable types with values of their own types
     func testNonOverwritable() throws {
         // make a deep copy so we start off with uniquely referenced storage
-        var doc = Document(fromPointer: DocumentTests.nonOverwritables.data)
+        var doc = Document(copying: DocumentTests.nonOverwritables.data)
 
         // save a reference to original bson_t so we can verify it changes
         var pointer = doc.data
@@ -578,7 +578,7 @@ final class DocumentTests: MongoSwiftTestCase {
     // test replacing both overwritable and nonoverwritable values with values of different types
     func testReplaceValueWithNewType() throws {
         // make a deep copy so we start off with uniquely referenced storage
-        var overwritableDoc = Document(fromPointer: DocumentTests.overwritables.data)
+        var overwritableDoc = Document(copying: DocumentTests.overwritables.data)
 
         // save a reference to original bson_t so we can verify it changes
         var overwritablePointer = overwritableDoc.data
@@ -613,7 +613,7 @@ final class DocumentTests: MongoSwiftTestCase {
         ]))
 
         // make a deep copy so we start off with uniquely referenced storage
-        var nonOverwritableDoc = Document(fromPointer: DocumentTests.nonOverwritables.data)
+        var nonOverwritableDoc = Document(copying: DocumentTests.nonOverwritables.data)
 
         // save a reference to original bson_t so we can verify it changes
         var nonOverwritablePointer = nonOverwritableDoc.data
@@ -631,7 +631,7 @@ final class DocumentTests: MongoSwiftTestCase {
 
     // test setting both overwritable and nonoverwritable values to nil
     func testReplaceValueWithNil() throws {
-        var overwritableDoc = Document(fromPointer: DocumentTests.overwritables.data)
+        var overwritableDoc = Document(copying: DocumentTests.overwritables.data)
         var overwritablePointer = overwritableDoc.data
 
         ["double", "int32", "int64", "bool", "decimal", "oid", "timestamp", "datetime"].forEach {
@@ -641,7 +641,7 @@ final class DocumentTests: MongoSwiftTestCase {
             overwritablePointer = overwritableDoc.data
         }
 
-        var nonOverwritableDoc = Document(fromPointer: DocumentTests.nonOverwritables.data)
+        var nonOverwritableDoc = Document(copying: DocumentTests.nonOverwritables.data)
         var nonOverwritablePointer = nonOverwritableDoc.data
 
         ["string", "doc", "arr"].forEach {