Bug 1197985 - Part 2 - Prevent potential overflows of the input buffer pointer when skipping large ID3 headers. r=esawin
authorJan Henning <jh+bugzilla@buttercookie.de>
Fri, 04 Sep 2015 18:00:09 +0200
changeset 261220 772e70e430885a69dca581ac647f7c1b6d0f54cc
parent 261219 afc2ca41d78b41c0126efa5116fc1bce690c4365
child 261221 ee49a5d7058cfe381655fafa39f290e0f6a07cd5
push id64692
push usercbook@mozilla.com
push dateTue, 08 Sep 2015 06:56:51 +0000
treeherdermozilla-inbound@ee49a5d7058c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersesawin
bugs1197985
milestone43.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1197985 - Part 2 - Prevent potential overflows of the input buffer pointer when skipping large ID3 headers. r=esawin
dom/media/MP3Demuxer.cpp
dom/media/MP3Demuxer.h
--- a/dom/media/MP3Demuxer.cpp
+++ b/dom/media/MP3Demuxer.cpp
@@ -399,25 +399,23 @@ MP3TrackDemuxer::FindNextFrame() {
          mOffset - mParser.ID3Header().Size() > MAX_SKIPPED_BYTES) ||
         (read = Read(buffer, mOffset, BUFFER_SIZE)) == 0) {
       // This is not a valid MPEG audio stream or we've reached EOS, give up.
       break;
     }
     NS_ENSURE_TRUE(mOffset + read > mOffset, MediaByteRange(0, 0));
     mOffset += read;
     bufferEnd = buffer + read;
-    frameBeg = mParser.Parse(buffer, bufferEnd);
+    const FrameParserResult parseResults = mParser.Parse(buffer, bufferEnd);
+    frameBeg = parseResults.mBufferPos;
 
-    if (frameBeg > bufferEnd) {
-      // We need to skip an ID3 tag which stretches beyond the current buffer.
-      const uint32_t bytesToSkip = frameBeg - bufferEnd;
-      NS_ENSURE_TRUE(mOffset + bytesToSkip > mOffset, MediaByteRange(0, 0));
-      mOffset += bytesToSkip;
-      frameBeg = bufferEnd;
-    }
+    // If mBytesToSkip is > 0, this skips the rest of an ID3 tag which stretches
+    // beyond the current buffer.
+    NS_ENSURE_TRUE(mOffset + parseResults.mBytesToSkip >= mOffset, MediaByteRange(0, 0));
+    mOffset += parseResults.mBytesToSkip;
   }
 
   if (frameBeg == bufferEnd || !mParser.CurrentFrame().Length()) {
     MP3DEMUXER_LOG("FindNext() Exit frameBeg=%p bufferEnd=%p "
                 "mParser.CurrentFrame().Length()=%d ",
                 frameBeg, bufferEnd, mParser.CurrentFrame().Length());
     return { 0, 0 };
   }
@@ -608,50 +606,55 @@ FrameParser::ID3Header() const {
   return mID3Parser.Header();
 }
 
 const FrameParser::VBRHeader&
 FrameParser::VBRInfo() const {
   return mVBRHeader;
 }
 
-const uint8_t*
+FrameParserResult
 FrameParser::Parse(const uint8_t* aBeg, const uint8_t* aEnd) {
   if (!aBeg || !aEnd || aBeg >= aEnd) {
-    return aEnd;
+    return { aEnd, 0 };
   }
 
   if (!mID3Parser.Header().Size() && !mFirstFrame.Length()) {
     // No MP3 frames have been parsed yet, look for ID3v2 headers at file begin.
     // ID3v1 tags may only be at file end.
     // TODO: should we try to read ID3 tags at end of file/mid-stream, too?
     const uint8_t* id3Beg = mID3Parser.Parse(aBeg, aEnd);
     if (id3Beg != aEnd) {
-      // ID3 headers found, skip past them.
-      aBeg = id3Beg + ID3Parser::ID3Header::SIZE + mID3Parser.Header().Size() +
-             mID3Parser.Header().FooterSize();
+      // ID3 tag found, skip past it.
+      const uint32_t tagSize = ID3Parser::ID3Header::SIZE + mID3Parser.Header().Size() +
+                               mID3Parser.Header().FooterSize();
+      const uint32_t remainingBuffer = aEnd - id3Beg;
+      if (tagSize > remainingBuffer) {
+        // Skipping across the ID3 tag would take us past the end of the buffer, therefore we
+        // return immediately and let the calling function handle skipping the rest of the tag.
+        return { aEnd, tagSize - remainingBuffer };
+      }
+      aBeg = id3Beg + tagSize;
     }
   }
 
   while (aBeg < aEnd && !mFrame.ParseNext(*aBeg)) {
     ++aBeg;
   }
 
   if (mFrame.Length()) {
     // MP3 frame found.
     if (!mFirstFrame.Length()) {
       mFirstFrame = mFrame;
     }
     // Move to the frame header begin to allow for whole-frame parsing.
     aBeg -= FrameHeader::SIZE;
+    return { aBeg, 0 };
   }
-  // If no headers (both ID3 and MP3) have been found, this is equivalent to returning aEnd.
-  // If we have found a large ID3 tag and want to skip past it, aBeg will point past the
-  // end of the buffer, which needs to be handled by the calling function.
-  return aBeg;
+  return { aEnd, 0 };
 }
 
 // FrameParser::Header
 
 FrameParser::FrameHeader::FrameHeader()
 {
   Reset();
 }
--- a/dom/media/MP3Demuxer.h
+++ b/dom/media/MP3Demuxer.h
@@ -106,16 +106,21 @@ public:
   // Resets the state to allow for a new parsing session.
   void Reset();
 
 private:
   // The currently parsed ID3 header. Reset via Reset, updated via Parse.
   ID3Header mHeader;
 };
 
+struct FrameParserResult {
+  const uint8_t* mBufferPos;
+  const uint32_t mBytesToSkip;
+};
+
 // MPEG audio frame parser.
 // The MPEG frame header has the following format (one bit per character):
 // 11111111 111VVLLC BBBBSSPR MMEETOHH
 // {   sync   } - 11 sync bits
 //   VV         - MPEG audio version ID (0->2.5, 1->reserved, 2->2, 3->1)
 //   LL         - Layer description (0->reserved, 1->III, 2->II, 3->I)
 //   C          - CRC protection bit (0->protected, 1->not protected)
 //   BBBB       - Bitrate index (see table in implementation)
@@ -280,21 +285,21 @@ public:
   void Reset();
 
   // Clear the last parsed frame to allow for next frame parsing, i.e.:
   // - sets PrevFrame to CurrentFrame
   // - resets the CurrentFrame
   // - resets ID3Header if no valid header was parsed yet
   void EndFrameSession();
 
-  // Parses given buffer [aBeg, aEnd) for a valid frame header.
-  // Returns begin of frame header if a frame header was found or a value >= aEnd otherwise.
-  // Values > aEnd indicate that additional bytes need to be skipped for jumping
-  // across an ID3 tag stretching beyond the given buffer.
-  const uint8_t* Parse(const uint8_t* aBeg, const uint8_t* aEnd);
+  // Parses given buffer [aBeg, aEnd) for a valid frame header and returns a FrameParserResult.
+  // FrameParserResult.mBufferPos points to begin of frame header if a frame header was found
+  // or to aEnd otherwise. FrameParserResult.mBytesToSkip indicates whether additional bytes need to
+  // be skipped in order to jump across an ID3 tag that stretches beyond the given buffer.
+  FrameParserResult Parse(const uint8_t* aBeg, const uint8_t* aEnd);
 
   // Parses given buffer [aBeg, aEnd) for a valid VBR header.
   // Returns whether a valid VBR header was found.
   bool ParseVBRHeader(const uint8_t* aBeg, const uint8_t* aEnd);
 
 private:
   // ID3 header parser.
   ID3Parser mID3Parser;