Put a bandaid on a segfault in timed audio track handling.

Add a bandaid to prevent a segfault which can occur while handling
timed audio buffers.  There is a deeper problem which should
eventually be addressed, but for now this fix should prevent any
crashing.

The deeper problem is as follows.

When the AudioFlinger mixer gets data to mix from an AudioTrack, it
ends up getting a structure filled out which points into an IMemory
region owned by the AudioTrack.  Unfortunately, this structure is not
holding a refcount on the IMemory which it points into.  If the
IMemory refcount hits 0 and the chunk of RAM is retuned to the binder
heap it came from, there can still be a Buffer object being held by
the AudioFlinger mixer which points into the region of memory which
was retuned to the binfer heap.  If AF reads from this buffer, it
could read corrupt data (if the region of memory gets handed back out
to a writer), or it could segfault (if the heap has been freed and the
pages unmapped).  Similar problems could happen if AF attempts to
write to the buffer, heap corruption in one case, segfaulting in the
other.

In the past, this has not been an issue for AF, because tracks
allocate a single IMemory (which serves as a ring buffer) and the
IMemory lives for as long as the track lives.  As an artifact of the
way the code came out, the mixer cannot be holding a Buffer structure
pointing into the IMemory which used to be owned by a track if the
track no longer exists.  Tracks cannot come into or out of existence
during a mix operation, which is the only thing which makes this safe.

TimedTracks work differently, however.  Timed tracks each allocate a
small binder heap, and then hand out IMemory instances  broken out of
this heap.  The heap lives as long as the track, so the worst which
could happen here is that a TimedTrack's IMemory gets returned to the
heap while there is still a buffer structure in flight pointing into
the memory region, then the region gets handed out again and
overwritten by new data causing the mixer to mix the wrong audio.  The
timing to cause this to happen is very difficult to encounter, and you
to generate the timing conditions required, you need to be in a pretty
bad failure state where audio is already breaking up and skipping, so
its unlikely that anyone would notice (which is why I'm band-aiding
the segfault and letting the deeper issue slide for now).

In general, however, it might be a good idea to revisit this buffering
design.  On principal, if someone is going to hold pointers into a
refcounted object, they should be holding a ref on the object at the
same time.  Failure to do this will usually lead to a situation where
there are corruption or segfault issues, or to a system where the
refcounted object's lifetime must be implicitly managed very carefully
in ways which are usually non-obvious and are easy to break by new
engineers on a project.

Change-Id: Ib391075395ed0ef46a03c37aa38a82d09e88abeb

1 files changed, 17 insertions, 4 deletions

diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp
index 29d63de..103e18b 100644
--- a/services/audioflinger/AudioFlinger.cpp
+++ b/services/audioflinger/AudioFlinger.cpp
@@ -4117,11 +4117,24 @@ void AudioFlinger::PlaybackThread::TimedTrack::releaseBuffer(
 
     Mutex::Autolock _l(mTimedBufferQueueLock);
 
-    if (buffer->raw != mTimedSilenceBuffer) {
+    // If the buffer which was just released is part of the buffer at the head
+    // of the queue, be sure to update the amt of the buffer which has been
+    // consumed.  If the buffer being returned is not part of the head of the
+    // queue, its either because the buffer is part of the silence buffer, or
+    // because the head of the timed queue was trimmed after the mixer called
+    // getNextBuffer but before the mixer called releaseBuffer.
+    if ((buffer->raw != mTimedSilenceBuffer) && mTimedBufferQueue.size()) {
         TimedBuffer& head = mTimedBufferQueue.editItemAt(0);
-        head.setPosition(head.position() + buffer->frameCount * mCblk->frameSize);
-        if (static_cast<size_t>(head.position()) >= head.buffer()->size()) {
-            mTimedBufferQueue.removeAt(0);
+
+        void* start = head.buffer()->pointer();
+        void* end   = head.buffer()->pointer() + head.buffer()->size();
+
+        if ((buffer->raw >= start) && (buffer->raw <= end)) {
+            head.setPosition(head.position() +
+                    (buffer->frameCount * mCblk->frameSize));
+            if (static_cast<size_t>(head.position()) >= head.buffer()->size()) {
+                mTimedBufferQueue.removeAt(0);
+            }
         }
     }