Possible race condition in SFBAudioPlayer when creating new _playerNode

[sbooth]

Since _playerNode may be swapped out in -configureProcessingGraphForFormat:forceUpdate:, there is a small chance that it could be nil when used in one of the other SFBAudioPlayer methods (e.g. -playbackPosition) if that method is called between the time _playerNode is set to nil and reassigned.

SFBAudioEngine/Sources/CSFBAudioEngine/Player/SFBAudioPlayer.mm

Line 946 in 02fa6a4

_playerNode = nil;

SFBAudioEngine/Sources/CSFBAudioEngine/Player/SFBAudioPlayer.mm

Line 447 in 02fa6a4

return _playerNode.playbackPosition;

SFBAudioEngine/Sources/CSFBAudioEngine/Player/SFBAudioPlayer.mm

Line 949 in 02fa6a4

_playerNode = playerNode;

The current implementation takes advantage of the fact that in Objective-C messaging nil isn't an error but the potential for incorrect behavior exists.

[jlama]

I believe a similar bug exists in AudioPlayerNode in all methods accessing the current decoder. For example:

SFBAudioEngine/Sources/CSFBAudioEngine/Player/AudioPlayerNode.mm

Lines 457 to 476 in e18876e

	SFBPlaybackTime SFB::AudioPlayerNode::PlaybackTime() const noexcept
	{
	const auto decoderState = GetActiveDecoderStateWithSmallestSequenceNumber();
	if(!decoderState)
	return SFBInvalidPlaybackTime;
	SFBPlaybackTime playbackTime = SFBInvalidPlaybackTime;
	const auto framePosition = decoderState->FramePosition();
	const auto frameLength = decoderState->FrameLength();
	if(const auto sampleRate = decoderState->mSampleRate; sampleRate > 0) {
	if(framePosition != SFBUnknownFramePosition)
	playbackTime.currentTime = framePosition / sampleRate;
	if(frameLength != SFBUnknownFrameLength)
	playbackTime.totalTime = frameLength / sampleRate;
	}
	return playbackTime;
	}

After the decoderState pointer is loaded, it could become invalid (deleted) at any time. Access and deletion of a DecoderState should probably be protected by a lock.

[sbooth]

I believe you're right. A lock isn't an option because of usage in the IOProc but additional synchronization is probably needed.

[jlama]

Ah yes indeed. I would argue that the render block should not fiddle with the decoder states and only report how many frames were rendered. Let the current decoder thread handle the frame accounting.
The drawback is that reporting start/stop render timings is more difficult, but I don't think these timings are very useful nor accurate (they don't account for downstream nodes latencies).