From d03d2ce8abff897d65be8ca46b631fe15c65f9df Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 29 Nov 2022 09:48:55 +1100 Subject: [PATCH] Fixed remaining issues, cleaned up memory & logic # Conflicts: # Session.xcodeproj/project.pbxproj --- Session.xcodeproj/project.pbxproj | 7 +- .../ConfigUserProfileSpec.swift | 82 +++++++++++-------- .../General/Collection+Subscripting.swift | 7 -- .../General/Collection+Utilities.swift | 22 +++++ .../General/String+Utilities.swift | 14 ++++ 5 files changed, 86 insertions(+), 46 deletions(-) delete mode 100644 SessionUtilitiesKit/General/Collection+Subscripting.swift create mode 100644 SessionUtilitiesKit/General/Collection+Utilities.swift diff --git a/Session.xcodeproj/project.pbxproj b/Session.xcodeproj/project.pbxproj index 6f25d8457..6e4f6dc62 100644 --- a/Session.xcodeproj/project.pbxproj +++ b/Session.xcodeproj/project.pbxproj @@ -254,7 +254,7 @@ B8DE1FB626C22FCB0079C9CE /* CallMessage.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8DE1FB526C22FCB0079C9CE /* CallMessage.swift */; }; B8EB20EE2640F28000773E52 /* VisibleMessage+OpenGroupInvitation.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8EB20ED2640F28000773E52 /* VisibleMessage+OpenGroupInvitation.swift */; }; B8EB20F02640F7F000773E52 /* OpenGroupInvitationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8EB20EF2640F7F000773E52 /* OpenGroupInvitationView.swift */; }; - B8F5F58325EC94A6003BF8D4 /* Collection+Subscripting.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8F5F58225EC94A6003BF8D4 /* Collection+Subscripting.swift */; }; + B8F5F58325EC94A6003BF8D4 /* Collection+Utilities.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8F5F58225EC94A6003BF8D4 /* Collection+Utilities.swift */; }; B8F5F60325EDE16F003BF8D4 /* DataExtractionNotification.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8F5F60225EDE16F003BF8D4 /* DataExtractionNotification.swift */; }; B8F5F71A25F1B35C003BF8D4 /* MediaPlaceholderView.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8F5F71925F1B35C003BF8D4 /* MediaPlaceholderView.swift */; }; B8FF8DAE25C0D00F004D1F22 /* SessionMessagingKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = C3C2A6F025539DE700C340D1 /* SessionMessagingKit.framework */; }; @@ -1371,7 +1371,7 @@ B8EB20E6263F7E4B00773E52 /* sk */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = sk; path = sk.lproj/Localizable.strings; sourceTree = ""; }; B8EB20ED2640F28000773E52 /* VisibleMessage+OpenGroupInvitation.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "VisibleMessage+OpenGroupInvitation.swift"; sourceTree = ""; }; B8EB20EF2640F7F000773E52 /* OpenGroupInvitationView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OpenGroupInvitationView.swift; sourceTree = ""; }; - B8F5F58225EC94A6003BF8D4 /* Collection+Subscripting.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Collection+Subscripting.swift"; sourceTree = ""; }; + B8F5F58225EC94A6003BF8D4 /* Collection+Utilities.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Collection+Utilities.swift"; sourceTree = ""; }; B8F5F60225EDE16F003BF8D4 /* DataExtractionNotification.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DataExtractionNotification.swift; sourceTree = ""; }; B8F5F71925F1B35C003BF8D4 /* MediaPlaceholderView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MediaPlaceholderView.swift; sourceTree = ""; }; B8FF8E6125C10DA5004D1F22 /* GeoLite2-Country-Blocks-IPv4 */ = {isa = PBXFileReference; lastKnownFileType = file.bplist; name = "GeoLite2-Country-Blocks-IPv4"; path = "Countries/GeoLite2-Country-Blocks-IPv4"; sourceTree = ""; }; @@ -2577,6 +2577,7 @@ B8F5F58225EC94A6003BF8D4 /* Collection+Subscripting.swift */, FDFD645A27F26D4600808CA1 /* Data+Utilities.swift */, FDC6D75F2862B3F600B04575 /* Dependencies.swift */, + FD26FA5D291CAFF9005801D8 /* Data+Utilities.swift */, C3C2A5D52553860A00C340D1 /* Dictionary+Utilities.swift */, B87EF18026377A1D00124B3C /* Features.swift */, B8BC00BF257D90E30032E807 /* General.swift */, @@ -5445,7 +5446,7 @@ FD52090028AF6153006098F6 /* OWSBackgroundTask.m in Sources */, C32C5DDB256DD9FF003C73A2 /* ContentProxy.swift in Sources */, C3A71F892558BA9F0043A11F /* Mnemonic.swift in Sources */, - B8F5F58325EC94A6003BF8D4 /* Collection+Subscripting.swift in Sources */, + B8F5F58325EC94A6003BF8D4 /* Collection+Utilities.swift in Sources */, C33FDEF8255A656D00E217F9 /* Promise+Delaying.swift in Sources */, 7BD477A827EC39F5004E2822 /* Atomic.swift in Sources */, B8BC00C0257D90E30032E807 /* General.swift in Sources */, diff --git a/SessionMessagingKitTests/LibSessionUtil/ConfigUserProfileSpec.swift b/SessionMessagingKitTests/LibSessionUtil/ConfigUserProfileSpec.swift index 6cd6a6adf..e08840408 100644 --- a/SessionMessagingKitTests/LibSessionUtil/ConfigUserProfileSpec.swift +++ b/SessionMessagingKitTests/LibSessionUtil/ConfigUserProfileSpec.swift @@ -2,6 +2,7 @@ import Foundation import SessionUtil +import SessionUtilitiesKit import Quick import Nimble @@ -11,11 +12,12 @@ class ConfigUserProfileSpec: QuickSpec { // MARK: - Spec override func spec() { - it("behaves correctly") { + it("generates configs correctly") { // Initialize a brand new, empty config because we have no dump data to deal with. let error: UnsafeMutablePointer? = nil var conf: UnsafeMutablePointer? = nil expect(user_profile_init(&conf, nil, 0, error)).to(equal(0)) + error?.deallocate() // We don't need to push anything, since this is an empty config expect(config_needs_push(conf)).to(beFalse()) @@ -23,7 +25,7 @@ class ConfigUserProfileSpec: QuickSpec { expect(config_needs_dump(conf)).to(beFalse()) // Since it's empty there shouldn't be a name. - let namePtr = user_profile_get_name(conf) + let namePtr: UnsafePointer? = user_profile_get_name(conf) expect(namePtr).to(beNil()) var toPush: UnsafeMutablePointer? = nil @@ -33,14 +35,14 @@ class ConfigUserProfileSpec: QuickSpec { let seqno: Int64 = config_push(conf, &toPush, &toPushLen) expect(toPush).toNot(beNil()) expect(seqno).to(equal(0)) - expect(String(cString: toPush!)).to(equal("d1:#i0e1:&de1:? = user_profile_get_name(conf) expect(namePtr2).toNot(beNil()) expect(String(cString: namePtr2!)).to(equal("Kallie")) - let pic2 = user_profile_get_pic(conf); + let pic2: user_profile_pic = user_profile_get_pic(conf); expect(pic2.url).toNot(beNil()) expect(pic2.key).toNot(beNil()) expect(pic2.keylen).to(equal(6)) expect(String(cString: pic2.url!)).to(equal("http://example.org/omg-pic-123.bmp")) - expect(String(cString: pic2.key!)).to(equal("secret")) + expect(String(pointer: pic2.key, length: pic2.keylen)).to(equal("secret")) // Since we've made changes, we should need to push new config to the swarm, *and* should need // to dump the updated state: @@ -80,7 +82,7 @@ class ConfigUserProfileSpec: QuickSpec { var toPush2: UnsafeMutablePointer? = nil var toPush2Len: Int = 0 - let seqno2 = config_push(conf, &toPush2, &toPush2Len); + let seqno2: Int64 = config_push(conf, &toPush2, &toPush2Len); // incremented since we made changes (this only increments once between // dumps; even though we changed two fields here). expect(seqno2).to(equal(1)) @@ -121,9 +123,9 @@ class ConfigUserProfileSpec: QuickSpec { ] .flatMap { $0 } - expect(String(cString: toPush2!)) - // Need to null-terminate the string or it'll crash - .to(equal(String(cString: expPush1.appending(CChar())))) + expect(String(pointer: toPush2, length: toPush2Len, encoding: .ascii)) + .to(equal(String(pointer: expPush1, length: expPush1.count, encoding: .ascii))) + toPush2?.deallocate() // We haven't dumped, so still need to dump: expect(config_needs_dump(conf)).to(beTrue()) @@ -155,8 +157,9 @@ class ConfigUserProfileSpec: QuickSpec { .map { CChar(bitPattern: $0) } ] .flatMap { $0 } - expect(String(cString: dump1!)) - .to(equal(String(cString: expDump1.appending(CChar()))))// Need to null-terminate the string or it'll crash + expect(String(pointer: dump1, length: dump1Len, encoding: .ascii)) + .to(equal(String(pointer: expDump1, length: expDump1.count, encoding: .ascii))) + dump1?.deallocate() // So now imagine we got back confirmation from the swarm that the push has been stored: config_confirm_pushed(conf, seqno2) @@ -167,7 +170,7 @@ class ConfigUserProfileSpec: QuickSpec { var dump2: UnsafeMutablePointer? = nil var dump2Len: Int = 0 config_dump(conf, &dump2, &dump2Len) - + dump2?.deallocate() expect(config_needs_dump(conf)).to(beFalse()) // Now we're going to set up a second, competing config object (in the real world this would be @@ -178,18 +181,14 @@ class ConfigUserProfileSpec: QuickSpec { var conf2: UnsafeMutablePointer? = nil expect(user_profile_init(&conf2, nil, 0, error2)).to(equal(0)) expect(config_needs_dump(conf2)).to(beFalse()) + error2?.deallocate() // Now imagine we just pulled down the `exp_push1` string from the swarm; we merge it into // conf2: - let mergeData: [[CChar]] = [expPush1] - var mergeDataPtr = mergeData.map { value in - let cStringCopy = UnsafeMutableBufferPointer.allocate(capacity: value.count) - _ = cStringCopy.initialize(from: value) - let ptr = UnsafePointer(cStringCopy.baseAddress) - return UnsafePointer(cStringCopy.baseAddress) - } + var mergeData: [UnsafePointer?] = [expPush1].unsafeCopy() var mergeSize: [Int] = [expPush1.count] - config_merge(conf2, &mergeDataPtr, &mergeSize, 1) + config_merge(conf2, &mergeData, &mergeSize, 1) + mergeData.forEach { $0?.deallocate() } // Our state has changed, so we need to dump: expect(config_needs_dump(conf2)).to(beTrue()) @@ -197,6 +196,7 @@ class ConfigUserProfileSpec: QuickSpec { var dump3Len: Int = 0 config_dump(conf2, &dump3, &dump3Len) // (store in db) + dump3?.deallocate() expect(config_needs_dump(conf2)).to(beFalse()) // We *don't* need to push: even though we updated, all we did is update to the merged data (and @@ -232,12 +232,12 @@ class ConfigUserProfileSpec: QuickSpec { expect(config_needs_push(conf2)).to(beTrue()) var toPush3: UnsafeMutablePointer? = nil var toPush3Len: Int = 0 - let seqno3 = config_push(conf, &toPush3, &toPush3Len) + let seqno3: Int64 = config_push(conf, &toPush3, &toPush3Len) expect(seqno3).to(equal(2)) // incremented, since we made a field change var toPush4: UnsafeMutablePointer? = nil var toPush4Len: Int = 0 - let seqno4 = config_push(conf2, &toPush4, &toPush4Len) + let seqno4: Int64 = config_push(conf2, &toPush4, &toPush4Len) expect(seqno4).to(equal(2)) // incremented, since we made a field change var dump4: UnsafeMutablePointer? = nil @@ -247,29 +247,34 @@ class ConfigUserProfileSpec: QuickSpec { var dump5Len: Int = 0 config_dump(conf2, &dump5, &dump5Len); // (store in db) + dump4?.deallocate() + dump5?.deallocate() // Since we set different things, we're going to get back different serialized data to be // pushed: - expect(String(cString: toPush3!)).toNot(equal(String(cString: toPush4!))) + expect(String(pointer: toPush3, length: toPush3Len, encoding: .ascii)) + .toNot(equal(String(pointer: toPush4, length: toPush4Len, encoding: .ascii))) // Now imagine that each client pushed its `seqno=2` config to the swarm, but then each client // also fetches new messages and pulls down the other client's `seqno=2` value. // Feed the new config into each other. (This array could hold multiple configs if we pulled // down more than one). - var mergeData2Ptr: [UnsafePointer?] = [UnsafePointer(toPush3)] + var mergeData2: [UnsafePointer?] = [UnsafePointer(toPush3)] var mergeSize2: [Int] = [toPush3Len] - config_merge(conf2, &mergeData2Ptr, &mergeSize2, 1) + config_merge(conf2, &mergeData2, &mergeSize2, 1) + toPush3?.deallocate() var mergeData3: [UnsafePointer?] = [UnsafePointer(toPush4)] var mergeSize3: [Int] = [toPush4Len] config_merge(conf, &mergeData3, &mergeSize3, 1) + toPush4?.deallocate() // Now after the merge we *will* want to push from both client, since both will have generated a // merge conflict update (with seqno = 3). expect(config_needs_push(conf)).to(beTrue()) expect(config_needs_push(conf2)).to(beTrue()) - let seqno5 = config_push(conf, &toPush3, &toPush3Len); - let seqno6 = config_push(conf2, &toPush4, &toPush4Len); + let seqno5: Int64 = config_push(conf, &toPush3, &toPush3Len); + let seqno6: Int64 = config_push(conf2, &toPush4, &toPush4Len); expect(seqno5).to(equal(3)) expect(seqno6).to(equal(3)) @@ -282,18 +287,16 @@ class ConfigUserProfileSpec: QuickSpec { // test). // Since only one of them set a profile pic there should be no conflict there: - let pic3 = user_profile_get_pic(conf) + let pic3: user_profile_pic = user_profile_get_pic(conf) expect(pic3.url).toNot(beNil()) expect(String(cString: pic3.url!)).to(equal("http://new.example.com/pic")) expect(pic3.key).toNot(beNil()) - expect(String(data: Data(bytes: pic3.key, count: pic3.keylen), encoding: .utf8)) - .to(equal("qwert\0yuio")) - let pic4 = user_profile_get_pic(conf2) + expect(String(pointer: pic3.key, length: pic3.keylen)).to(equal("qwert\0yuio")) + let pic4: user_profile_pic = user_profile_get_pic(conf2) expect(pic4.url).toNot(beNil()) expect(String(cString: pic4.url!)).to(equal("http://new.example.com/pic")) expect(pic4.key).toNot(beNil()) - expect(String(data: Data(bytes: pic4.key, count: pic4.keylen), encoding: .utf8)) - .to(equal("qwert\0yuio")) + expect(String(pointer: pic4.key, length: pic4.keylen)).to(equal("qwert\0yuio")) config_confirm_pushed(conf, seqno5) config_confirm_pushed(conf2, seqno6) @@ -305,11 +308,18 @@ class ConfigUserProfileSpec: QuickSpec { var dump7Len: Int = 0 config_dump(conf2, &dump7, &dump7Len); // (store in db) + dump6?.deallocate() + dump7?.deallocate() expect(config_needs_dump(conf)).to(beFalse()) expect(config_needs_dump(conf2)).to(beFalse()) expect(config_needs_push(conf)).to(beFalse()) expect(config_needs_push(conf2)).to(beFalse()) + + // Wouldn't do this in a normal session but doing it here to properly clean up + // after the test + conf?.deallocate() + conf2?.deallocate() } } } diff --git a/SessionUtilitiesKit/General/Collection+Subscripting.swift b/SessionUtilitiesKit/General/Collection+Subscripting.swift deleted file mode 100644 index 70ffcf61d..000000000 --- a/SessionUtilitiesKit/General/Collection+Subscripting.swift +++ /dev/null @@ -1,7 +0,0 @@ - -extension Collection { - - public subscript(ifValid index: Index) -> Iterator.Element? { - return self.indices.contains(index) ? self[index] : nil - } -} diff --git a/SessionUtilitiesKit/General/Collection+Utilities.swift b/SessionUtilitiesKit/General/Collection+Utilities.swift new file mode 100644 index 000000000..1c1954c26 --- /dev/null +++ b/SessionUtilitiesKit/General/Collection+Utilities.swift @@ -0,0 +1,22 @@ +// Copyright © 2022 Rangeproof Pty Ltd. All rights reserved. + +import Foundation + +extension Collection { + public subscript(ifValid index: Index) -> Iterator.Element? { + return self.indices.contains(index) ? self[index] : nil + } +} + +public extension Collection where Element == [CChar] { + /// This creates an array of UnsafePointer types to access data of the C strings in memory. This array provides no automated + /// memory management of it's children so after use you are responsible for handling the life cycle of the child elements and + /// need to call `deallocate()` on each child. + func unsafeCopy() -> [UnsafePointer?] { + return self.map { value in + let copy = UnsafeMutableBufferPointer.allocate(capacity: value.count) + _ = copy.initialize(from: value) + return UnsafePointer(copy.baseAddress) + } + } +} diff --git a/SessionUtilitiesKit/General/String+Utilities.swift b/SessionUtilitiesKit/General/String+Utilities.swift index 39bbbb913..be3bd36ad 100644 --- a/SessionUtilitiesKit/General/String+Utilities.swift +++ b/SessionUtilitiesKit/General/String+Utilities.swift @@ -74,6 +74,20 @@ public extension String { } } +// MARK: - Convenience + +public extension String { + /// Initialize with an optional pointer and a spoecific length + init?(pointer: UnsafeRawPointer?, length: Int, encoding: String.Encoding = .utf8) { + guard + let pointer: UnsafeRawPointer = pointer, + let result: String = String(data: Data(bytes: pointer, count: length), encoding: encoding) + else { return nil } + + self = result + } +} + // MARK: - Formatting public extension String {