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

App Crashes due to null value in _Nonnull parameter in YubiKit 4.2.0 #94

Closed
WWSellers opened this issue Mar 25, 2022 · 1 comment · Fixed by #96
Closed

App Crashes due to null value in _Nonnull parameter in YubiKit 4.2.0 #94

WWSellers opened this issue Mar 25, 2022 · 1 comment · Fixed by #96

Comments

@WWSellers
Copy link

WWSellers commented Mar 25, 2022

The error argument value can still be nil (null) despite being declared _Nonnull

@optional
- (void)didFailConnectingNFC:(NSError *_Nonnull)error;

Swift version

func didFailConnectingNFC(_ error: Error)

Reproduction:

  • Use YubiKit version 4.2.0 (Did not test version 4.1.0)
  • App starts a connection to both Lightning (MFI) and NFC.
  • When the user connects an MFI key, the app will call YubiKitManager method stopNFCConnection()
  • YubiKitManager calls the delegate method's didFailConnectingNFC() with nil argument.
  • When app tries to use error or its members, the app crashes with EXC_BAD_ACCESS (code=1, address=0x0)

Workaround
Update on workaround: this only works when SWIFT_OPTIMIZATION_LEVEL = "-Onone". The optimizer will remove the code when set to "-O" for release builds.

App must check if error == nil. If it is, then assume that there is no error, just the app calling stopNFCConnection().
However, the Swift compiler gives a warning:

Comparing non-optional value of type 'Error' to 'nil' always returns false

If a developer is treating warnings as errors, then they will need to make exceptions. (And deal with their security departments.)

Recommendation
Specifiy _Nullable for the parameter

Other Notes

The YKFNFCConnection member nfcConnectionErroris declared _Nullable,

@property (nonatomic, readonly, nullable) NSError *nfcConnectionError;

but is passed to the delegate (YubiKitManager)

[self.delegate didFailConnectingNFC:self.nfcConnectionError];

YubiKitManager.m implementation does not declare _Nullable or _Nonnull for error

- (void)didFailConnectingNFC:(NSError *)error {

but YubiKitManager.h interface does declare _Nonnull

@optional
- (void)didFailConnectingNFC:(NSError *_Nonnull)error;

(Adding _Nonnull to the implementation did not cause the compiler to give an error)

- (void)didFailConnectingNFC:(NSError *_Nonnull)error {

Environment
Xcode 13.3
iPhone 13 with iOS 15.4
YubiKit 4.2.0

@WWSellers WWSellers changed the title nil/null value in non-nil parameter in YubiKit 4.2.0 App Crashes due to nil/null value in non-nil parameter in YubiKit 4.2.0 Apr 12, 2022
@WWSellers WWSellers changed the title App Crashes due to nil/null value in non-nil parameter in YubiKit 4.2.0 App Crashes due to null value in _Nonnull parameter in YubiKit 4.2.0 Apr 12, 2022
@jensutbult
Copy link
Contributor

Yes, this should never happen. The fix in #96 makes sure we only call the didFailConnectingNFC method when there's an actual error to pass on. The reasoning for not passing on an error when stopNFCConnection has been called is that the NFC connection closing without connecting to a YubiKey is expected behaviour at that point.

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

Successfully merging a pull request may close this issue.

2 participants