MacPass / KeePassKit

KeePass Database loading, storing and manipulation framework
Other
125 stars 40 forks source link

IOS: add binary crash. #35

Closed kaich closed 6 years ago

kaich commented 6 years ago

I add photo to my entry as an attachment!

let binary = KPKBinary(name: name, data: data)
entry.addBinary(attachment)

when save tree. It crash at self.tree!.encryp!

mstarke commented 6 years ago

Could you be a bit more specific? What format does the image have? Any error traces? Does encryption work without the attachment?

mstarke commented 6 years ago

The crash is particularly strange since there is a test case that passes on iOS. See:

https://github.com/MacPass/KeePassKit/blob/b3fb8f135ef04cf0f751b4e7ce5cd8c2dcc25e84/KeePassKitTests/KPKTextKdbxWriting.m#L1-L66

kaich commented 6 years ago

@mstarke How do I add argon2 when I use cocoapods ? It's simple to use carthage in my project. But I don't know how add keepasskit with cocoapods. If I can add Keepasskit with cocoapods. I can figure out where crash!

mstarke commented 6 years ago

Why do you need to add argon2 with CocoaPods? It's a submodule, doesn't CocoaPods support submodules in git repos?

Do you need CocoaPods support to be able to debug KeePassKit?

kaich commented 6 years ago

@mstarke Yes, I need CocoaPods support to be able to debug KeePassKit. I add KeePassKit as below : pod 'KeePassKit', :git => 'https://github.com/MacPass/KeePassKit.git'. But Argon2 and KissXML don't installed with cocoapods.

mstarke commented 6 years ago

Mh. Might this help you in any way?

http://allocinit.io/ios/debugging-carthage-dependencies/

kaich commented 6 years ago

@mstarke The Crash trace as blew:

2018-03-27 3 50 56

it crash at - (NSData *)kpk_hashedSha256Data. I only pick a image from album. Then get the image data. Save the photo data as a KPKBinary. I don't know the reason of crash. It only crash when a pick a image as KPKBinary(Attachment)!

mstarke commented 6 years ago

Do you have any log messages or just a crash? Are you able to step through the hashing? Does is crash for every picture you use? How big is the file?

mstarke commented 6 years ago

I'll try to replicate the setup.

kaich commented 6 years ago

@mstarke The last called method: [self getBytes:slice range:NSMakeRange(location, blockLength)]; in kpk_hashedSha256Data . Then it crash. Thanks!

kaich commented 6 years ago

When I change KPKTree minimumVersion to kKPKKdbxFileVersion4, It works fine.

mstarke commented 6 years ago

You discovered a nice API bug. minimumVersion should never be a writeable property. What confused me is that the actual setter does not change the return value of KPKTree.minimumVersionor did you patch it?

The consequence is, that the base64 encoding seems to be broken since this is not happening when you use KDBX4 as format.

kaich commented 6 years ago

I only call - (void)setValue:(id)value forPublicCustomDataKey:(NSString *)key; method. So it doesn't call kpk_hashedSha256Data method and it works fine.

mstarke commented 6 years ago

Please keep this issue open until it is fixed.

mstarke commented 6 years ago

Does the crash only happen on an iOS device?

kaich commented 6 years ago

I have found the reason. I think it's stack overflow. When I change NSUInteger blockSize = 1024*1024; to NSUInteger blockSize = 20*1024; in kpk_hashedSha256Data method. It works fine.

0b8391e5-be68-4767-8c99-4f47e575ea58

Is it a reason of crash ?

mstarke commented 6 years ago

Yup. I wanted to wait for your answer to verify my suspicions. I'll switch to malloc since iOS seems to have problems with the size is the array.

kaich commented 6 years ago

@mstarke I think you can change to heap from stack. Also you need change the size of block. If you set 1024*1024, It's so slow. When I change to 20*1024, It's so fast. And kpk_unhashedSha256Data also has same problem. Hope you fix it.

mstarke commented 6 years ago

I think it's even worth trying to parallelise the operation. I did this to the xoring for NSData and it sped up a lot and since iDevices have multiple cores this might bring some more benefits. There are a couple of places where I need to be carefull with sack sizes. I did fix some already.

mstarke commented 6 years ago

Fixed by switching to heap allocation in 583c1044fe29bab99a90a52dda3dbefae66e44b8 and published as 1.13.4. The buffer size is kept the same. If you would find the time to introduce plattform dependant optimisations for the default buffer size it would be awesome if you can open a Pull Request. Thanks for pointing this out!

jbs-fm commented 5 years ago

@mstarke Yes, I need CocoaPods support to be able to debug KeePassKit. I add KeePassKit as below : pod 'KeePassKit', :git => 'https://github.com/MacPass/KeePassKit.git'. But Argon2 and KissXML don't installed with cocoapods.

Hi @kaich, are you be able to share the lines in your PodFile that get KissXML/Argon2 installed? Would be greatly appreciated.