diff --git a/Sources/MongoSwift/APM.swift b/Sources/MongoSwift/APM.swift index 225e89cae..15b5f2769 100644 --- a/Sources/MongoSwift/APM.swift +++ b/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)) } } diff --git a/Sources/MongoSwift/BSON/BSONValue.swift b/Sources/MongoSwift/BSON/BSONValue.swift index 22089fddb..eee2f0ff3 100644 --- a/Sources/MongoSwift/BSON/BSONValue.swift +++ b/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) } diff --git a/Sources/MongoSwift/BSON/Document.swift b/Sources/MongoSwift/BSON/Document.swift index 3b9936473..ea7ef8494 100644 --- a/Sources/MongoSwift/BSON/Document.swift +++ b/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) in + self.storage = DocumentStorage(stealing: try fromJSON.withUnsafeBytes { (bytes: UnsafePointer) 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) in + self.storage = DocumentStorage(stealing: fromBSON.withUnsafeBytes { (bytes: UnsafePointer) 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) } } } diff --git a/Sources/MongoSwift/MongoClient.swift b/Sources/MongoSwift/MongoClient.swift index c3bc6e724..0c7ef8945 100644 --- a/Sources/MongoSwift/MongoClient.swift +++ b/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 { diff --git a/Sources/MongoSwift/MongoCursor.swift b/Sources/MongoSwift/MongoCursor.swift index 233959ddb..ccbe075f8 100644 --- a/Sources/MongoSwift/MongoCursor.swift +++ b/Sources/MongoSwift/MongoCursor.swift @@ -80,7 +80,10 @@ public class MongoCursor: Sequence, IteratorProtocol { } var replyPtr = UnsafeMutablePointer.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: 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: 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 diff --git a/Sources/MongoSwift/ReadPreference.swift b/Sources/MongoSwift/ReadPreference.swift index 042e358a7..5a8c043ef 100644 --- a/Sources/MongoSwift/ReadPreference.swift +++ b/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] diff --git a/Sources/MongoSwift/SDAM.swift b/Sources/MongoSwift/SDAM.swift index c4e5d080e..282737785 100644 --- a/Sources/MongoSwift/SDAM.swift +++ b/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. diff --git a/Tests/MongoSwiftTests/DocumentTests.swift b/Tests/MongoSwiftTests/DocumentTests.swift index 91e123711..e8fc2c056 100644 --- a/Tests/MongoSwiftTests/DocumentTests.swift +++ b/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 {