Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unarchive(objectForKey:) should throw #17

Closed
alexeylubyanoy opened this issue Sep 18, 2019 · 9 comments
Closed

unarchive(objectForKey:) should throw #17

alexeylubyanoy opened this issue Sep 18, 2019 · 9 comments

Comments

@alexeylubyanoy
Copy link

alexeylubyanoy commented Sep 18, 2019

Could you please change

public func unarchive(objectForKey key:String) -> Any? {
        guard let data = self.data(forKey: key) else {
            return nil
        }

        let unarchiver = NSKeyedUnarchiver(forReadingWith: data as Data)
        return unarchiver.decodeObject(forKey: key)
}

to

public func unarchive(objectForKey key:String) -> Any? throws {
        guard let data = self.data(forKey: key) else {
            return nil
        }

        let unarchiver = NSKeyedUnarchiver(forReadingWith: data as Data)
        return unarchiver.decodeTopLevelObject(forKey: key)

It allows to catch the exception in case of failure to decode data.

In my case module name was changed (it's being used as kind of workspace) and I got crash
when trying to decode the object encoded with old module name.

Error Domain=NSCocoaErrorDomain Code=4864 "*** -[NSKeyedUnarchiver decodeObjectForKey:]: cannot decode object of class (MyOldTargetName.MyClass) for key (root); the class may be defined in source code or a library that is not linked" UserInfo={NSDebugDescription=*** -[NSKeyedUnarchiver decodeObjectForKey:]: cannot decode object of class (MyOldTargetName.MyClass) for key (root); the class may be defined in source code or a library that is not linked}

Thanks in advance.

@granoff
Copy link
Owner

granoff commented Sep 19, 2019

I am not sure that making this api throw is the solution to your issue. Does the class MyOldTargetName (or whatever your class is named) confirm to NSSecureCoding?

@granoff
Copy link
Owner

granoff commented Sep 19, 2019

I Just re-read what you wrote. Yah, if you change the name of the class your trying to decode, that's a problem in your code. Making this api throw is a breaking change, meaning it would be incompatible with previous versions. I'll have to think about this more carefully.

@alexeylubyanoy
Copy link
Author

@granoff Another option may be to leave existing method w/o any changes and just introduce another with throw. WDYT?

@granoff
Copy link
Owner

granoff commented Sep 19, 2019

I think I'd rather add a new API that throws, instead of changing this existing API. That would not break existing implementations...

@bharathi91
Copy link
Contributor

bharathi91 commented Oct 14, 2019

@granoff
Am also facing the same issue.

Below is the trace.

0 libsystem_malloc.dylib nanov2_allocate_from_block.cold.1 + 40
1 libsystem_malloc.dylib nanov2_find_block_and_allocate + 554
2 libsystem_malloc.dylib nanov2_allocate + 128
3 libsystem_malloc.dylib nanov2_calloc + 140
4 libsystem_malloc.dylib default_zone_calloc + 84
5 libsystem_malloc.dylib malloc_zone_calloc + 148
6 CoreFoundation CFAllocatorAllocate + 108
7 CoreFoundation _CFRuntimeCreateInstance + 340
8 CoreFoundation __CFDataInit + 168
9 Security add_sequence_to_array + 76
10 CoreFoundation -[__NSDictionaryM __apply:context:] + 132
11 Security der_encode_dictionary + 108
12 Security SecXPCDictionarySetPList + 100
13 Security securityd_send_sync_and_do + 72
14 Security __SecItemCopyMatching_block_invoke_2 + 264
15 Security __SecItemAuthDoQuery_block_invoke + 320
16 Security SecItemAuthDo + 140
17 Security SecItemAuthDoQuery + 520
18 Security __SecItemCopyMatching_block_invoke + 120
19 Security SecOSStatusWith + 56
20 Security SecItemCopyMatching + 364
21 Strongbox Strongbox.swift line 152Strongbox.data(forKey:) + 152
22 Strongbox Strongbox.swift line 95Strongbox.unarchive(objectForKey:) + 95

Is there any fix for this?

@granoff
Copy link
Owner

granoff commented Oct 14, 2019

@alexeylubyanoy Can you give me a little more detail about what your code looked like before you changed the module name, and then what the code looked like after you changed the module name and you got the crash you reported?

I am trying to write some unit tests to replicate the issue before I try to change any of the APIs in the library.

@alexeylubyanoy
Copy link
Author

alexeylubyanoy commented Oct 15, 2019

import Security
import Strongbox
import Foundation

@objc class Credentials: NSObject, NSSecureCoding {
    static var supportsSecureCoding: Bool = true

    func encode(with aCoder: NSCoder) {
        aCoder.encode(username, forKey: "username")
        aCoder.encode(password, forKey: "password")
    }

    required init?(coder aDecoder: NSCoder) {
        guard
            let username = aDecoder.decodeObject(forKey: "username") as? String,
            let password = aDecoder.decodeObject(forKey: "password") as? String else {
            return nil
        }

        self.username = username
        self.password = password

        super.init()
    }

    @objc let username: String
    @objc let password: String

    @objc init(with username: String, password: String) {
        self.username = username
        self.password = password
    }
}

@objc class KeychainHelper: NSObject {
    @discardableResult
    @objc class func store(credentials: Credentials) -> Bool {
        KeychainHelper.removeCredentials()

        return Strongbox().archive(credentials, key: "defaultUser")
    }

    @objc class var storedCredentials: Credentials? {
        return Strongbox().unarchive(objectForKey: "defaultUser") as? Credentials
    }
}

So, nothing special here.

Issue appears when I call following code with old target name

let creds = Credentials(with: "username", password: "password")
KeychainHelper.store(creds)

and then change target name to the new one and call

let creds = KeychainHelper.storedCredentials

Please let me know if you need more details.
My workaround so far is to modify project settings to hardcode module name, so module name remains despite target name changed

@granoff
Copy link
Owner

granoff commented Oct 15, 2019

A couple more thoughts. The bundle identifier is integral to storing the data in the keychain if you do not specify a key prefix yourself. So if your target changed, then the bundle identifier changed. You can get around this by initializing your Strongbox instance with a keyPrefix argument:
Strongbox(keyPrefix: <the-old-bundle-identifier>). That might solve your problem.

I'm also curious about what your KeychainHelper.removeCredentials() method is doing. Strongbox has a convenience remove method for this purpose, and understands the key prefix stuff too.

@alexeylubyanoy
Copy link
Author

@discardableResult
    @objc class func removeCredentials() -> Bool {
        return Strongbox().remove(key: "defaultUser")
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants