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

media/filters/skcanvas_video_renderer.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/skcanvas_video_renderer.h"
 
 #include "base/logging.h"
 #include "media/base/video_frame.h"
 #include "media/base/yuv_convert.h"
 #include "third_party/libyuv/include/libyuv.h"
@@ skipping 190 matching lines @@
     }
     default:
       NOTREACHED();
       break;
   }
 }
 
 // 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 {
     info->fWidth = frame_->visible_rect().width();
     info->fHeight = frame_->visible_rect().height();
     info->fColorType = kN32_SkColorType;
     info->fAlphaType = kPremul_SkAlphaType;
     return true;
   }
 
   virtual bool onGetPixels(const SkImageInfo& info,
                            void* pixels,
                            size_t row_bytes,
                            SkPMColor ctable[],
                            int* ctable_count) OVERRIDE {
-    if (!frame_.get())
+    // This method should be called only one time.
+    DCHECK(frame_.get());
+    if (!pixels)
       return false;
-    if (!pixels)
-      return true;
     // If skia couldn't do the YUV conversion, we will.
     ConvertVideoFrameToRGBPixels(frame_, pixels, row_bytes);
-    frame_ = NULL;
     return true;
   }
 
   virtual bool onGetYUV8Planes(SkISize sizes[3],
                                void* planes[3],
                                size_t row_bytes[3],
                                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()))
         *color_space = kJPEG_SkYUVColorSpace;
       else
         *color_space = kRec601_SkYUVColorSpace;
     }
 
     for (int plane = VideoFrame::kYPlane; plane <= VideoFrame::kVPlane;
          ++plane) {
@@ skipping 12 matching lines @@
         if (plane == media::VideoFrame::kYPlane) {
           offset = (frame_->stride(media::VideoFrame::kYPlane) *
                     frame_->visible_rect().y()) +
                    frame_->visible_rect().x();
         } else {
           offset = (frame_->stride(media::VideoFrame::kUPlane) *
                     (frame_->visible_rect().y() >> y_shift)) +
                    (frame_->visible_rect().x() >> 1);
         }
         row_bytes[plane] = static_cast<size_t>(frame_->stride(plane));
         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;
   }
 
  private:
   scoped_refptr<VideoFrame> frame_;
 };
 
 SkCanvasVideoRenderer::SkCanvasVideoRenderer()
-    : generator_(NULL), last_frame_timestamp_(media::kNoTimestamp()) {
+    : last_frame_timestamp_(media::kNoTimestamp()) {
   last_frame_.setIsVolatile(true);
 }
 
 SkCanvasVideoRenderer::~SkCanvasVideoRenderer() {}
 
 void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
                                   SkCanvas* canvas,
                                   const gfx::RectF& dest_rect,
                                   uint8 alpha,
                                   SkXfermode::Mode mode,
                                   VideoRotation video_rotation) {
   if (alpha == 0) {
     return;
   }
 
   SkRect dest;
   dest.set(dest_rect.x(), dest_rect.y(), dest_rect.right(), dest_rect.bottom());
 
   SkPaint paint;
   paint.setAlpha(alpha);
 
   // Paint black rectangle if there isn't a frame available or the
   // frame has an unexpected format.
   if (!video_frame.get() || !IsYUVOrNative(video_frame->format())) {
     canvas->drawRect(dest, paint);
     canvas->flush();
     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();
     }
 
     // TODO(rileya): Perform this rotation on the canvas, rather than allocating
     // a new bitmap and copying.
     switch (video_rotation) {
       case VIDEO_ROTATION_0:
         break;
       case VIDEO_ROTATION_90:
         last_frame_ = SkBitmapOperations::Rotate(
             last_frame_, SkBitmapOperations::ROTATION_90_CW);
         break;
       case VIDEO_ROTATION_180:
         last_frame_ = SkBitmapOperations::Rotate(
             last_frame_, SkBitmapOperations::ROTATION_180_CW);
         break;
       case VIDEO_ROTATION_270:
         last_frame_ = SkBitmapOperations::Rotate(
             last_frame_, SkBitmapOperations::ROTATION_270_CW);
         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);
 
   // Paint using |last_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,
                                  SkCanvas* canvas) {
   Paint(video_frame,
         canvas,
         video_frame->visible_rect(),
         0xff,
         SkXfermode::kSrc_Mode,
         media::VIDEO_ROTATION_0);
 }
 
 }  // namespace media