[Gnash-commit] /srv/bzr/gnash/trunk r10694: Pad buffer correctly in FLV parser, fixing illegal reads and uninitialized

[Benjamin Wolsey]

------------------------------------------------------------
revno: 10694
committer: Benjamin Wolsey <address@hidden>
branch nick: trunk
timestamp: Fri 2009-03-13 14:44:20 +0100
message:
  Pad buffer correctly in FLV parser, fixing illegal reads and uninitialized
  values in parsing some FLVs. It's still not at all exception-safe, but much
  tidier.
modified:
  libmedia/FLVParser.cpp
  libmedia/FLVParser.h
    ------------------------------------------------------------
    revno: 10691.1.6
    committer: Benjamin Wolsey <address@hidden>
    branch nick: work
    timestamp: Fri 2009-03-13 13:06:31 +0100
    message:
      Pad input data properly (i.e. up to the end of the buffer) and set frame 
size
      to the number of bytes actually read. Before, the frame size was set to 
the
      number of bytes requested, regardless of how much was read, and only 64 
bytes
      (the padding) were set to 0, leaving the end of the buffer uninitialized
      while inviting the media handler to read it.
      
      Eight bytes of padding should now be sufficient.
    modified:
      libmedia/FLVParser.cpp
      libmedia/FLVParser.h

=== modified file 'libmedia/FLVParser.cpp'
--- a/libmedia/FLVParser.cpp    2009-02-11 21:58:37 +0000
+++ b/libmedia/FLVParser.cpp    2009-03-13 12:06:31 +0000
@@ -33,8 +33,6 @@
 #include <string>
 #include <iosfwd>
 
-
-#define PADDING_BYTES 64
 #define READ_CHUNKS 64
 
 // The amount of bytes to scan an FLV for
@@ -74,7 +72,9 @@
 
 }
 
-const boost::uint16_t FLVParser::FLVAudioTag::flv_audio_rates [] = {5500, 
11000, 22050, 44100};
+const size_t FLVParser::paddingBytes;
+const boost::uint16_t FLVParser::FLVAudioTag::flv_audio_rates [] = 
+    { 5500, 11000, 22050, 44100 };
 
 FLVParser::FLVParser(std::auto_ptr<IOChannel> lt)
        :
@@ -228,12 +228,20 @@
        {
                _audioInfo.reset(new AudioInfo(audiotag.codec, 
audiotag.samplerate,
                     audiotag.samplesize, audiotag.stereo, 0, FLASH) );
-               if (header) {
-                       boost::uint8_t* newbuf = new 
boost::uint8_t[frame->dataSize];
-                       memcpy(newbuf, frame->data.get(), frame->dataSize);
+
+        if (header) {
+
+            // The frame is 0-padded up to the end. It may be larger than
+            // this if fewer bytes were read than requested, but it is
+            // never smaller.
+            const size_t bufSize = frame->dataSize + paddingBytes;
+
+            boost::uint8_t* data = new boost::uint8_t[bufSize];
+
+            std::copy(frame->data.get(), frame->data.get() + bufSize, data);
 
                        _audioInfo->extra.reset( 
-                               new ExtraAudioInfoFlv(newbuf, frame->dataSize)
+                               new ExtraAudioInfoFlv(data, frame->dataSize)
                        );
 
                        // The FAAD decoder will reject us if we pass the 
header buffer.
@@ -294,14 +302,19 @@
        // If this is the first videoframe no info about the
        // video format has been noted, so we do that now
        if ( ! _videoInfo.get() ) {
-               _videoInfo.reset( new VideoInfo(videotag.codec, 0 /* width */, 
0 /* height */, 0 /*frameRate*/, 0 /*duration*/, FLASH /*codec type*/) );
+               _videoInfo.reset(new VideoInfo(videotag.codec, 0, 0, 0, 0, 
FLASH));
 
                if (header) {
-                       boost::uint8_t* newbuf = new 
boost::uint8_t[frame->dataSize()];
-                       memcpy(newbuf, frame->data(), frame->dataSize());
-
+            // The frame is 0-padded up to the end. It may be larger than
+            // this if fewer bytes were read than requested, but it is
+            // never smaller.
+            const size_t bufSize = frame->dataSize() + paddingBytes;
+
+            boost::uint8_t* data = new boost::uint8_t[bufSize];
+
+            std::copy(frame->data(), frame->data() + bufSize, data);
                        _videoInfo->extra.reset( 
-                               new ExtraVideoInfoFlv(newbuf, frame->dataSize())
+                               new ExtraVideoInfoFlv(data, frame->dataSize())
                        );
 
                        // Don't bother emitting the header buffer.
@@ -538,27 +551,24 @@
 std::auto_ptr<EncodedAudioFrame>
 FLVParser::readAudioFrame(boost::uint32_t dataSize, boost::uint32_t timestamp)
 {
-       //log_debug("Reading the %dth audio frame, with data size %d, from 
position %d", _audioFrames.size()+1, dataSize, _stream->tell());
-
-       std::auto_ptr<EncodedAudioFrame> frame ( new EncodedAudioFrame );
-       frame->dataSize = dataSize;
+
+       std::auto_ptr<EncodedAudioFrame> frame(new EncodedAudioFrame);
+    
+    const size_t bufSize = dataSize + paddingBytes;
+
+    boost::uint8_t* data = new boost::uint8_t[bufSize];
+       const size_t bytesRead = _stream->read(data, dataSize);
+
+    std::fill(data + bytesRead, data + bufSize, 0);
+
+       if (bytesRead < dataSize) {
+               log_error("FLVParser::readAudioFrame: could only read %d/%d 
bytes",
+                bytesRead, dataSize);
+       }
+
+       frame->dataSize = bytesRead;
        frame->timestamp = timestamp;
-
-       const size_t chunkSize = smallestMultipleContaining(READ_CHUNKS,
-            dataSize + PADDING_BYTES);
-
-       frame->data.reset(new boost::uint8_t[chunkSize]);
-
-       const size_t bytesread = _stream->read(frame->data.get(), dataSize);
-       if ( bytesread < dataSize )
-       {
-               log_error("FLVParser::readAudioFrame: could only read %d/%d 
bytes",
-                bytesread, dataSize);
-       }
-
-       const size_t padding = chunkSize - dataSize;
-       assert(padding);
-    std::fill_n(frame->data.get() + bytesread, padding, 0);
+       frame->data.reset(data);
 
        return frame;
 }
@@ -570,21 +580,18 @@
 {
        std::auto_ptr<EncodedVideoFrame> frame;
 
-       const size_t chunkSize = smallestMultipleContaining(READ_CHUNKS,
-            dataSize + PADDING_BYTES);
-
-       boost::uint8_t* data = new boost::uint8_t[chunkSize];
-       size_t bytesread = _stream->read(data, dataSize);
-
-       const size_t padding = chunkSize - dataSize;
-       assert(padding);
-    std::fill_n(data + bytesread, padding, 0);
+    const size_t bufSize = dataSize + paddingBytes;
+
+       boost::uint8_t* data = new boost::uint8_t[bufSize];
+       const size_t bytesRead = _stream->read(data, dataSize);
+
+    std::fill(data + bytesRead, data + bufSize, 0);
 
        // We won't need frameNum, so will set to zero...
        // TODO: fix this ?
        // NOTE: ownership of 'data' is transferred here
 
-       frame.reset(new EncodedVideoFrame(data, dataSize, 0, timestamp));
+       frame.reset(new EncodedVideoFrame(data, bytesRead, 0, timestamp));
        return frame;
 }
 
@@ -606,5 +613,4 @@
 } // end of gnash::media namespace
 } // end of gnash namespace
 
-#undef PADDING_BYTES
 #undef READ_CHUNKS 

=== modified file 'libmedia/FLVParser.h'
--- a/libmedia/FLVParser.h      2009-01-22 20:10:39 +0000
+++ b/libmedia/FLVParser.h      2009-03-13 12:06:31 +0000
@@ -107,6 +107,13 @@
 
 public:
 
+    /// The size of padding for all buffers that might be read by FFMPEG.
+    //
+    /// This possibly also applies to other media handlers (gst).
+    /// Ideally this would be taken from the current Handler, but we don't
+    /// know about it here.
+    static const size_t paddingBytes = 8;
+
        /// \brief
        /// Create an FLV parser reading input from
        /// the given IOChannel