Skip to content
This repository has been archived by the owner on Jun 2, 2018. It is now read-only.

Fix: KVO Observation blocks can get called with garbage parameters #276

Merged
merged 2 commits into from
Nov 4, 2014

Conversation

ipmcc
Copy link
Contributor

@ipmcc ipmcc commented Nov 4, 2014

Short version: In the KVO addObserver methods that take an options parameter, if the caller passes 0, the observation block can get called with fewer parameters than expected, which in ARC, causes the compiler to try to retain what is effectively garbage, causing a hard crash.

Longer version: For instance, if I create an observation like this:

[self bk_addObserverForKeyPaths: @"keypath" options:(NSKeyValueObservingOptions)0 task:^(id obj, NSString *keyPath, NSDictionary *change) {
    // We will crash here when `keypath` changes causing a notification
}];

that call will get passed down to -[NSObject bk_addObserverForKeyPaths:identifier:options:task:] which determines what context to use with this line:

    BKObserverContext context = (options == 0) ? BKObserverContextManyKeys : BKObserverContextManyKeysWithChange;

except that in -[_BKObserver observeValueForKeyPath:ofObject:change:context:], the notification having a context of BKObserverContextManyKeys (instead of BKObserverContextManyKeysWithChange) will cause the task block to get casted to void (^task)(id, NSString *), when the block that was passed in was a void (^task)(id, NSString *, NSDictionary *). The result is that when task is called, the third parameter is garbage (whatever happens to be at that position in the stack.) ARC emits calls that assumes that all parameters are valid or nil, so a lot of the time (but not always, if the garbage value happens to be a valid ObjC object) this will cause a hard crash.

To avoid this, it would be advisable to always call blocks with the signature that they expect, regardless of options (which this patch does.) Alternatively, you could assert when someone calls the options:-taking methods with 0 for options, but I'm partial to the solution here.

I filed an Issue related to this pull request: #277

…tionary gets called with one, even if the caller didn't include any options that would warrant a change dictionary being sent.
@zwaldowski
Copy link
Collaborator

Brilliant fix. Since NSKeyValueObservingOptions is an NSUInteger, can we get a slightly higher mask value? I don't think Apple's likely to change it any time soon, but better safe than sorry.

@ipmcc
Copy link
Contributor Author

ipmcc commented Nov 4, 2014

Updated to use 0x1000

@zwaldowski
Copy link
Collaborator

Thanks.

zwaldowski added a commit that referenced this pull request Nov 4, 2014
Fix: KVO Observation blocks can get called with garbage parameters
@zwaldowski zwaldowski merged commit d6f282b into BlocksKit:master Nov 4, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants