zwaldowski/BlocksKit — pull request #276 by ipmcc (2014-11-04 15:51:48+00:00)
Title: Fix: KVO Observation blocks can get called with garbage parameters

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.