Fix potential use-after-free bug in VideoImageGenerator::onGetYUV8Planes.

media/filters/skcanvas_video_renderer.cc

Index: media/filters/skcanvas_video_renderer.cc
diff --git a/media/filters/skcanvas_video_renderer.cc b/media/filters/skcanvas_video_renderer.cc
index 9bdd393e56c31c368b926de08be9e4e42a993382..752dfffa862ecaf73fe1975aba1b098d6d1c9701 100644
--- a/media/filters/skcanvas_video_renderer.cc
+++ b/media/filters/skcanvas_video_renderer.cc
@@ -208,10 +208,13 @@ static void ConvertVideoFrameToRGBPixels(
 // Generates an RGB image from a VideoFrame.
 class VideoImageGenerator : public SkImageGenerator {
  public:
-  VideoImageGenerator(const scoped_refptr<VideoFrame>& frame) : frame_(frame) {}
+  VideoImageGenerator(const scoped_refptr<VideoFrame>& frame) : frame_(frame) {
+    DCHECK(frame_.get());
+  }
   virtual ~VideoImageGenerator() {}
-
-  void set_frame(const scoped_refptr<VideoFrame>& frame) { frame_ = frame; }
+  void discard_frame() {
+    frame_ = NULL;
+  }
 
  protected:
   virtual bool onGetInfo(SkImageInfo* info) OVERRIDE {
@@ -227,13 +230,12 @@ class VideoImageGenerator : public SkImageGenerator {
                            size_t row_bytes,
                            SkPMColor ctable[],
                            int* ctable_count) OVERRIDE {
-    if (!frame_.get())
-      return false;
+    // This method should be called only one time.
+    DCHECK(frame_.get());
     if (!pixels)
-      return true;
+      return false;
     // If skia couldn't do the YUV conversion, we will.
     ConvertVideoFrameToRGBPixels(frame_, pixels, row_bytes);
-    frame_ = NULL;
     return true;
   }
 
@@ -243,6 +245,11 @@ class VideoImageGenerator : public SkImageGenerator {
                                SkYUVColorSpace* color_space) OVERRIDE {
     if (!frame_.get() || !IsYUV(frame_->format()))
       return false;
+#if DCHECK_IS_ON
+    // This method should be called to get a YUV plane only one time.
+    if (row_bytes && planes)
+      DCHECK(frame_.get());
+#endif
 
     if (color_space) {
       if (IsJPEGColorSpace(frame_->format()))
@@ -278,8 +285,6 @@ class VideoImageGenerator : public SkImageGenerator {
         planes[plane] = frame_->data(plane) + offset;

[dshwang, 2014/10/02 18:23:52]

It exposes VideoFrame's data to caller

       }
     }
-    if (planes && row_bytes)
-      frame_ = NULL;

[dshwang, 2014/10/02 18:23:52]

Removing frame_ can cause seg fault. So I remove |frame_| after
SkCanvas::flush()

     return true;
   }
 
@@ -288,7 +293,7 @@ class VideoImageGenerator : public SkImageGenerator {
 };
 
 SkCanvasVideoRenderer::SkCanvasVideoRenderer()
-    : generator_(NULL), last_frame_timestamp_(media::kNoTimestamp()) {
+    : last_frame_timestamp_(media::kNoTimestamp()) {
   last_frame_.setIsVolatile(true);
 }
 
@@ -318,13 +323,13 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
     return;
   }
 
+  VideoImageGenerator* generator = NULL;
   // Check if we should convert and update |last_frame_|.
   if (last_frame_.isNull() ||
       video_frame->timestamp() != last_frame_timestamp_) {
-    generator_ = new VideoImageGenerator(video_frame);
-
-    // Note: This takes ownership of |generator_|.
-    if (!SkInstallDiscardablePixelRef(generator_, &last_frame_)) {
+    generator = new VideoImageGenerator(video_frame);
+    // Note: This takes ownership of new VideoImageGenerator instance.
+    if (!SkInstallDiscardablePixelRef(generator, &last_frame_)) {
       NOTREACHED();
     }
 
@@ -347,15 +352,7 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
         break;
     }
 
-    // We copied the frame into a new bitmap and threw out the old one, so we
-    // no longer have a |generator_| around. This should be removed when the
-    // above TODO is addressed.
-    if (video_rotation != VIDEO_ROTATION_0)
-      generator_ = NULL;
-
     last_frame_timestamp_ = video_frame->timestamp();
-  } else if (generator_) {
-    generator_->set_frame(video_frame);
   }
 
   paint.setXfermodeMode(mode);
@@ -364,6 +361,10 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
   paint.setFilterLevel(SkPaint::kLow_FilterLevel);
   canvas->drawBitmapRect(last_frame_, NULL, dest, &paint);
   canvas->flush();
+  // SkCanvas::flush() causes the generator to generate SkImage, so delete
+  // |video_frame| not to be outlived.
+  if (generator)
+    generator->discard_frame();

[dshwang, 2014/10/09 19:59:57]

This line caused use-after-free because SkCanvas::flush() can delete |generator|
already, so remove it.

[dshwang, 2014/10/10 14:13:55]

It's wrong analysis. |generator| is deleted by replacing SkBitmap as follows:
last_frame_ = SkBitmapOperations::Rotate(last_frame_ =
SkBitmapOperations::Rotate(last_frame_, SkBitmapOperations::ROTATION_90_CW);
However replacement code is deleted in
https://codereview.chromium.org/619343003/, so this line is resurrectted

 }
 
 void SkCanvasVideoRenderer::Copy(const scoped_refptr<VideoFrame>& video_frame,