Open Radar

 
Before iOS 13, `AVAudioPlayer` declares its delegate as `assign`. With iOS 13 and later, `AVAudioPlayer` declares its delegate as `weak`, but still internally accesses the delegate directly via a raw pointer instead of using `objc_loadWeak()`.

This causes a crash when `AVAudioPlayer` invokes a method on the delegate which internally releases the last reference to the delegate (e.g. invalidating `CADisplayLink` or `NSTimer`).

Note: Disconnecting the delegate in `-dealloc` doesn't work around this bug, since we're already in a delegate method by the time `-dealloc` is called.

This reproduces on iOS 13.3 as well as macOS 10.15.3.

Repro: Compile and run the attached file `AVAudioPlayerCrash.m` for macOS or iOS with `NSZombieEnabled=YES` set in the environment:

```
% clang -fobjc-arc -fobjc-abi-version=2 -framework Foundation -framework AVFoundation -ObjC -o AVAudioPlayerCrash AVAudioPlayerCrash.m
% NSZombieEnabled=YES ./AVAudioPlayerCrash
2020-03-09 10:22:18.531 AVAudioPlayerCrash[5121:6485407] Created audio player <AudioPlayerBug: 0x7fbbc2e02da0>
2020-03-09 10:22:18.531 AVAudioPlayerCrash[5121:6485407] Starting timer
2020-03-09 10:22:18.531 AVAudioPlayerCrash[5121:6485407] Playing audio
2020-03-09 10:22:18.753 AVAudioPlayerCrash[5121:6485407] Audio player did finish playing, successfully: 1
2020-03-09 10:22:18.753 AVAudioPlayerCrash[5121:6485407] Invalidating timer
2020-03-09 10:22:18.753 AVAudioPlayerCrash[5121:6485407] Deallocating audio player <AudioPlayerBug: 0x7fbbc2e02da0>
2020-03-09 10:22:18.753 AVAudioPlayerCrash[5121:6485407] Finished invalidating timer (we will crash now if self was deallocated)
2020-03-09 10:22:18.753 AVAudioPlayerCrash[5121:6485407] *** -[AudioPlayerBug class]: message sent to deallocated instance 0x7fbbc2e02da0
[1]    5121 illegal hardware instruction  NSZombieEnabled=YES ./AVAudioPlayerCrash
```

Expected Behavior:

App should not crash when `AVAudioPlayer` invokes a delegate method which releases the last reference to the delegate.

Actual Behavior:

App crashes 100% when `AVAudioPlayer` invokes a delegate method which releases the last reference to the delegate.

Root cause:

Because `AVAudioPlayer`'s delegate is accessed directly via an ivar instead of via `objc_loadWeak()`, ARC does not retain a reference to the delegate for the duration of the delegate method. If that delegate method touches `self` after it's been deallocated, it will crash with `SIGSEGV`.

Proposed fix:

`AVAudioPlayer` should access its delegate using `objc_loadWeak()` or a similar explicit strong reference.

In addition, `-[AVAudioPlayer setDelegate:]` should use `objc_storeWeak()` to store the delegate instead of copying a raw pointer.

Workaround:

Use `NS_VALID_UNTIL_END_OF_SCOPE` from within the delegate method before invalidating any timers or display links which might release the last reference to `self`:

```
- (void)audioPlayerDidFinishPlaying:(AVAudioPlayer *)audioPlayer successfully:(BOOL)flag {
  NS_VALID_UNTIL_END_OF_SCOPE __typeof__(self) strongSelf = self;
  // snip
}
```

Alternately, do not reference `self` in an `AVAudioPlayerDelegate` method after any point which might release the last reference to `self` (e.g. invalidating `NSTimer` or `CADisplayLink`).

% cat AVAudioPlayer.m

#import <Foundation/Foundation.h>
#import <AVFoundation/AVFoundation.h>

// Sample MP3 data, Base64 encoded.
NSString *const kMP3DataBase64 =
    @"/+MYxAAAAANIAAAAAExBTUUzLjk4LjIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
    @"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";

@interface AudioPlayerBug : NSObject <AVAudioPlayerDelegate> {
  AVAudioPlayer *_audioPlayer;
  NSTimer *_timer;
}

- (void)play;

@end

@implementation AudioPlayerBug

- (instancetype)init {
  self = [super init];
  if (self) {
    NSData *audioData = [[NSData alloc] initWithBase64EncodedString:kMP3DataBase64 options:0];
    NSAssert(audioData != nil, @"Audio data must be valid");
    NSError *error = nil;
    _audioPlayer = [[AVAudioPlayer alloc] initWithData:audioData
                                          fileTypeHint:@"public.mp3"
                                                 error:&error];
    NSAssert(_audioPlayer != nil, @"Audio player must be valid");
    NSAssert(error == nil, @"Got error: %@", error);
    _audioPlayer.delegate = self;
  }
  NSLog(@"Created audio player %@", self);
  return self;
}

- (void)dealloc {
  NSLog(@"Deallocating audio player %@", self);

  // Before iOS 13, the delegate is declared `assign`, so we must manually disconnect the delegate
  // in `-dealloc` to ensure `AVAudioPlayer` doesn't access this object after `self` is deallocated.
  //
  // For iOS 13 and later, this should not be necessary, since the delegate is declared `weak`.
  // However, even in iOS 13, `AVAudioPlayer` incorrectly accesses the delegate via a raw pointer
  // instead of using `objc_loadWeak()`.  That means we might get here while we're in the middle of
  // a delegate method, at which point it's too late to disconnect the delegate.
  _audioPlayer.delegate = nil;
}

- (void)play {
  NSLog(@"Starting timer");
  _timer = [NSTimer scheduledTimerWithTimeInterval:1
                                            target:self
                                          selector:@selector(onTimer:)
                                          userInfo:nil
                                           repeats:NO];
  NSLog(@"Playing audio");
  [_audioPlayer play];
}

- (void)audioPlayerDidFinishPlaying:(AVAudioPlayer *)player successfully:(BOOL)successfully {

  NSLog(@"Audio player did finish playing, successfully: %d", successfully);

  // BUG: This releases the last reference to `self`.
  //
  // Root cause: `AVAudioPlayer` does not use `objc_loadWeak()` to load the reference to its
  // delegate, but instead accesses it via a raw pointer, which means ARC does not hold a
  // reference to the delegate for the lifetime of this method.
  NSAssert(_audioPlayer != nil, @"Audio player must not be deallocated");

  NSLog(@"Invalidating timer");
  [_timer invalidate];
  NSLog(@"Finished invalidating timer (we will crash now if self was deallocated)");

  // This assertion check will fail if ARC has just deallocated self.
  NSAssert(_audioPlayer != nil, @"Audio player must not be deallocated (self=%@)", self);
}

- (void)onTimer:(NSTimer *)timer {
  NSLog(@"Timer fired");
}

@end

int main(int argc, char **argv) {
  @autoreleasepool {
    __attribute((objc_precise_lifetime)) AudioPlayerBug *bug = [[AudioPlayerBug alloc] init];
    [bug play];
  }
  [NSRunLoop.mainRunLoop run];
  return 0;
}