Remove memset from VideoFrame and mark buffer as unpoisoned

media/filters/ffmpeg_video_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/ffmpeg_video_decoder.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 109 matching lines @@
       RoundUp(std::max(size.width(), codec_context->coded_width), 2),
       RoundUp(std::max(size.height(), codec_context->coded_height), 2));
 
   if (!VideoFrame::IsValidConfig(format, VideoFrame::STORAGE_UNKNOWN,
                                  coded_size, gfx::Rect(size), natural_size)) {
     return AVERROR(EINVAL);
   }
 
   scoped_refptr<VideoFrame> video_frame = frame_pool_.CreateFrame(
       format, coded_size, gfx::Rect(size), natural_size, kNoTimestamp());
+  // FFmpeg expects the initialize allocation to be zero-initialized.  Failure
+  // to do so can lead to unitialized value usage.  See http://crbug.com/390941
+  const size_t data_size = VideoFrame::AlignedAllocationSize(video_frame);
+  memset(video_frame->data(0), 0, data_size);

[DaleCurtis, 2015/07/09 21:35:11]

We don't want to do this here actually, as now memset() is called for every
video frame instead of for every new video frame.  Instead we should have an
option for the VideoFramePool to do this on initial allocation only.

[emircan, 2015/07/09 22:12:18]

Done. I added |zero_out_memory| bool to carry that information to the creation. 

+
   if (codec_context->colorspace == AVCOL_SPC_BT709) {
     video_frame->metadata()->SetInteger(VideoFrameMetadata::COLOR_SPACE,
                                         VideoFrame::COLOR_SPACE_HD_REC709);
   }
 
-  for (int i = 0; i < 3; i++) {
+  for (size_t i = 0; i < VideoFrame::NumPlanes(video_frame->format()); i++) {
     frame->data[i] = video_frame->data(i);
     frame->linesize[i] = video_frame->stride(i);
   }
 
   frame->width = coded_size.width();
   frame->height = coded_size.height();
   frame->format = codec_context->pix_fmt;
   frame->reordered_opaque = codec_context->reordered_opaque;
 
   // Now create an AVBufferRef for the data just allocated. It will own the
   // reference to the VideoFrame object.
   void* opaque = NULL;
   video_frame.swap(reinterpret_cast<VideoFrame**>(&opaque));
   frame->buf[0] =
       av_buffer_create(frame->data[0],
-                       VideoFrame::AllocationSize(format, coded_size),

[DaleCurtis, 2015/07/09 21:36:22]

Hmm, I think we were intentionally using the lower size here such that ffmpeg
didn't think it had further padding areas to read/write from (which it does in
many places).

[emircan, 2015/07/09 22:12:17]

I see. It looked like a mistake to me in the beginning as frame->data[] points
to the aligned&padded buffers and av_buffer_create goes for the packed size,
which is a mismatch. 
If we are not using such padded areas, should we remove them from allocation and
only allocate the packed size instead?

[DaleCurtis, 2015/07/09 22:21:16]

Hmm, I see, we can try it your way and see what falls out.

[emircan, 2015/07/09 23:16:27]

Done.

[emircan, 2015/07/10 18:43:40]

Well, bots showed that it didn't work out. Reverting this.

+                       data_size,
                        ReleaseVideoBufferImpl,
                        opaque,
                        0);
   return 0;
 }
 
 std::string FFmpegVideoDecoder::GetDisplayName() const {
   return "FFmpegVideoDecoder";
 }
 
@@ skipping 191 matching lines @@
   if (!codec || avcodec_open2(codec_context_.get(), codec, NULL) < 0) {
     ReleaseFFmpegResources();
     return false;
   }
 
   av_frame_.reset(av_frame_alloc());
   return true;
 }
 
 }  // namespace media