Skip to content

Commit

Permalink
Fix a buffer overflow issue by working around a bug in Core Audio whe…
Browse files Browse the repository at this point in the history
…re an unit is requested more bytes of audio than the maximum.

inp->pullInput ended up writing outside the bounds of the input buffer because CoreAudio requested 528 frames instead of 512... Go figure. This was observed in macOS 10.14.6 build 18G6020.
Obviously this could cause crashes, even though in practice they seem rare (one of such crashes may have been posted in error in a comment to issue #4165).
All cores which require an audio format conversion are affected (one of them is Gambatte).

The fix is more range checking. I also increased the effective input buffer size, to avoid giving CoreAudio less bytes than it requests -- at least in the case I have seen.
  • Loading branch information
shysaur committed Aug 11, 2020
1 parent aeb0da4 commit 3092582
Showing 1 changed file with 23 additions and 9 deletions.
32 changes: 23 additions & 9 deletions OpenEmu/OEAudioUnit.mm
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@

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

#include <algorithm>
#import "OEAudioUnit.h"
#import "OELogging.h"

typedef struct {
AURenderPullInputBlock pullInput;
AudioTimeStamp const *timestamp;
void **buffer;
UInt32 *bufferSizeBytes;
UInt32 *bufferSizeFrames;
UInt32 *bytesPerFrame;
} inputData;

static OSStatus audioConverterComplexInputDataProc(AudioConverterRef inAudioConverter, AVAudioPacketCount * ioNumberDataPackets, AudioBufferList * ioData, AudioStreamPacketDescription * __nullable * __nullable ioDataPacketDescription, void * inUserData)
Expand All @@ -40,7 +42,12 @@ static OSStatus audioConverterComplexInputDataProc(AudioConverterRef inAudioConv

AudioUnitRenderActionFlags pullFlags = 0;
ioData->mBuffers[0].mData = *inp->buffer;
ioData->mBuffers[0].mDataByteSize = *inp->bufferSizeBytes;
ioData->mBuffers[0].mDataByteSize = (*inp->bufferSizeFrames) * (*inp->bytesPerFrame);

/* cap the bytes we return to the amount of bytes available to guard
* against core audio requesting more bytes that they fit in the buffer
* EVEN THOUGH THE BUFFER IS ALREADY LARGER THAN MAXIMUMFRAMESTORENDER */
*ioNumberDataPackets = std::min(*inp->bufferSizeFrames, *ioNumberDataPackets);

return inp->pullInput(&pullFlags, inp->timestamp, *ioNumberDataPackets, 0, ioData);
}
Expand All @@ -51,7 +58,8 @@ @interface CustomBus: AUAudioUnitBus
@interface OEAudioUnit () {
AudioConverterRef _conv;
void *_convBuffer;
UInt32 _convSizeBytes;
UInt32 _convInputFrameCount;
UInt32 _convInputBytePerFrame;
}

@property (nonatomic, readonly) BOOL requiresConversion;
Expand Down Expand Up @@ -101,7 +109,6 @@ - (instancetype)initWithComponentDescription:(AudioComponentDescription)componen
_outputBusArray = [[AUAudioUnitBusArray alloc] initWithAudioUnit:self
busType:AUAudioUnitBusTypeOutput
busses: @[_outputBus]];


self.maximumFramesToRender = 512;

Expand Down Expand Up @@ -143,8 +150,14 @@ - (BOOL)allocateRenderResourcesAndReturnError:(NSError *__autoreleasing _Nullab
NSLog(@"unable to create audio converter: %d", status);
return nil;
}
_convSizeBytes = (UInt32)(srcDesc->mBytesPerFrame * self.maximumFramesToRender);
_convBuffer = malloc(_convSizeBytes);
/* 64 bytes of padding above self.maximumFramesToRender because
* CoreAudio is stupid and likes to request more bytes than the maximum
* even though IT TAKES CARE TO SET THE MAXIMUM VALUE ITSELF! */
_convInputFrameCount = (UInt32)self.maximumFramesToRender + 64;
_convInputBytePerFrame = (UInt32)srcDesc->mBytesPerFrame;
UInt32 bufferSize = _convInputFrameCount * _convInputBytePerFrame;
_convBuffer = malloc(_convInputFrameCount * _convInputBytePerFrame);
os_log_error(OE_LOG_AUDIO, "audio converter buffer size = %{public}u bytes", bufferSize);

return YES;
}
Expand All @@ -155,7 +168,7 @@ - (void)deallocateRenderResources {
}

- (void)_freeResources {
_convSizeBytes = 0;
_convInputFrameCount = 0;
if (_convBuffer) {
free(_convBuffer);
_convBuffer = nil;
Expand All @@ -177,7 +190,8 @@ - (AUInternalRenderBlock)internalRenderBlock {

__block inputData data = {
.buffer = &_convBuffer,
.bufferSizeBytes = &_convSizeBytes,
.bufferSizeFrames = &_convInputFrameCount,
.bytesPerFrame = &_convInputBytePerFrame,
.pullInput = _outputProvider,
};

Expand Down

0 comments on commit 3092582

Please sign in to comment.