Bug 1190210 - Part 1: Avoid wrong memory accessing in CropAndCopyDataSourceSurface(). r=smaug
authorKaku Kuo <tkuo@mozilla.com>
Wed, 05 Aug 2015 17:46:47 +0800
changeset 258208 9a8c950d08effc16b9caf23e1fa39b4c1e1d077e
parent 258207 88df6aa8bf026a090a16e44bc1f31975965eb69b
child 258209 d25a897f1e9d5981d291b1e316ae2978066f0068
push id29246
push userkwierso@gmail.com
push dateTue, 18 Aug 2015 22:23:21 +0000
treeherdermozilla-central@db4616cd0721 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1190210
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 1190210 - Part 1: Avoid wrong memory accessing in CropAndCopyDataSourceSurface(). r=smaug
dom/canvas/ImageBitmap.cpp
--- a/dom/canvas/ImageBitmap.cpp
+++ b/dom/canvas/ImageBitmap.cpp
@@ -27,60 +27,125 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Im
 NS_IMPL_CYCLE_COLLECTING_ADDREF(ImageBitmap)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(ImageBitmap)
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ImageBitmap)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 
 /*
+ * If either aRect.width or aRect.height are negative, then return a new IntRect
+ * which represents the same rectangle as the aRect does but with positive width
+ * and height.
+ */
+static IntRect
+FixUpNegativeDimension(const IntRect& aRect, ErrorResult& aRv)
+{
+  gfx::IntRect rect = aRect;
+
+  // fix up negative dimensions
+  if (rect.width < 0) {
+    CheckedInt32 checkedX = CheckedInt32(rect.x) + rect.width;
+
+    if (!checkedX.isValid()) {
+      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
+      return rect;
+    }
+
+    rect.x = checkedX.value();
+    rect.width = -(rect.width);
+  }
+
+  if (rect.height < 0) {
+    CheckedInt32 checkedY = CheckedInt32(rect.y) + rect.height;
+
+    if (!checkedY.isValid()) {
+      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
+      return rect;
+    }
+
+    rect.y = checkedY.value();
+    rect.height = -(rect.height);
+  }
+
+  return rect;
+}
+
+/*
  * This helper function copies the data of the given DataSourceSurface,
  *  _aSurface_, in the given area, _aCropRect_, into a new DataSourceSurface.
  * This might return null if it can not create a new SourceSurface or it cannot
  * read data from the given _aSurface_.
+ *
+ * Warning: Even though the area of _aCropRect_ is just the same as the size of
+ *          _aSurface_, this function still copy data into a new
+ *          DataSourceSurface.
  */
 static already_AddRefed<DataSourceSurface>
-CropDataSourceSurface(DataSourceSurface* aSurface, const IntRect& aCropRect)
+CropAndCopyDataSourceSurface(DataSourceSurface* aSurface, const IntRect& aCropRect)
 {
   MOZ_ASSERT(aSurface);
 
+  // Check the aCropRect
+  ErrorResult error;
+  const IntRect positiveCropRect = FixUpNegativeDimension(aCropRect, error);
+  if (NS_WARN_IF(error.Failed())) {
+    return nullptr;
+  }
+
   // Calculate the size of the new SourceSurface.
-  const SurfaceFormat format = aSurface->GetFormat();
-  const IntSize  dstSize = gfx::IntSize(aCropRect.width, aCropRect.height);
-  const uint32_t dstStride = dstSize.width * BytesPerPixel(format);
+  // We cannot keep using aSurface->GetFormat() to create new DataSourceSurface,
+  // since it might be SurfaceFormat::B8G8R8X8 which does not handle opacity,
+  // however the specification explicitly define that "If any of the pixels on
+  // this rectangle are outside the area where the input bitmap was placed, then
+  // they will be transparent black in output."
+  // So, instead, we force the output format to be SurfaceFormat::B8G8R8A8.
+  const SurfaceFormat format = SurfaceFormat::B8G8R8A8;
+  const int bytesPerPixel = BytesPerPixel(format);
+  const IntSize dstSize = IntSize(positiveCropRect.width,
+                                  positiveCropRect.height);
+  const uint32_t dstStride = dstSize.width * bytesPerPixel;
 
   // Create a new SourceSurface.
   RefPtr<DataSourceSurface> dstDataSurface =
-    Factory::CreateDataSourceSurfaceWithStride(dstSize, format, dstStride);
+    Factory::CreateDataSourceSurfaceWithStride(dstSize, format, dstStride, true);
 
   if (NS_WARN_IF(!dstDataSurface)) {
     return nullptr;
   }
 
-  // Copy the raw data into the newly created DataSourceSurface.
-  RefPtr<DataSourceSurface> srcDataSurface = aSurface;
-  DataSourceSurface::MappedSurface srcMap;
-  DataSourceSurface::MappedSurface dstMap;
-  if (NS_WARN_IF(!srcDataSurface->Map(DataSourceSurface::MapType::READ, &srcMap)) ||
-      NS_WARN_IF(!dstDataSurface->Map(DataSourceSurface::MapType::WRITE, &dstMap))) {
-    return nullptr;
-  }
+  // Only do copying and cropping when the positiveCropRect intersects with
+  // the size of aSurface.
+  const IntRect surfRect(IntPoint(0, 0), aSurface->GetSize());
+  if (surfRect.Intersects(positiveCropRect)) {
+    const IntRect surfPortion = surfRect.Intersect(positiveCropRect);
+    const IntPoint dest(std::max(0, surfPortion.X() - positiveCropRect.X()),
+                        std::max(0, surfPortion.Y() - positiveCropRect.Y()));
 
-  uint8_t* srcBufferPtr = srcMap.mData + aCropRect.y * srcMap.mStride
-                                       + aCropRect.x * BytesPerPixel(format);
-  uint8_t* dstBufferPtr = dstMap.mData;
-  for (int i = 0; i < dstSize.height; ++i) {
-    memcpy(dstBufferPtr, srcBufferPtr, dstMap.mStride);
-    srcBufferPtr += srcMap.mStride;
-    dstBufferPtr += dstMap.mStride;
+    // Copy the raw data into the newly created DataSourceSurface.
+    DataSourceSurface::ScopedMap srcMap(aSurface, DataSourceSurface::READ);
+    DataSourceSurface::ScopedMap dstMap(dstDataSurface, DataSourceSurface::WRITE);
+    if (NS_WARN_IF(!srcMap.IsMapped()) ||
+        NS_WARN_IF(!dstMap.IsMapped())) {
+      return nullptr;
+    }
+
+    uint8_t* srcBufferPtr = srcMap.GetData() + surfPortion.y * srcMap.GetStride()
+                                             + surfPortion.x * bytesPerPixel;
+    uint8_t* dstBufferPtr = dstMap.GetData() + dest.y * dstMap.GetStride()
+                                             + dest.x * bytesPerPixel;
+    const uint32_t copiedBytesPerRaw = surfPortion.width * bytesPerPixel;
+
+    for (int i = 0; i < surfPortion.height; ++i) {
+      memcpy(dstBufferPtr, srcBufferPtr, copiedBytesPerRaw);
+      srcBufferPtr += srcMap.GetStride();
+      dstBufferPtr += dstMap.GetStride();
+    }
   }
 
-  srcDataSurface->Unmap();
-  dstDataSurface->Unmap();
-
   return dstDataSurface.forget();
 }
 
 /*
  * Encapsulate the given _aSurface_ into a layers::CairoImage.
  */
 static already_AddRefed<layers::Image>
 CreateImageFromSurface(SourceSurface* aSurface, ErrorResult& aRv)
@@ -124,17 +189,17 @@ CreateSurfaceFromRawData(const gfx::IntS
     return nullptr;
   }
 
   // The temporary cropRect variable is equal to the size of source buffer if we
   // do not need to crop, or it equals to the given cropping size.
   const IntRect cropRect = aCropRect.valueOr(IntRect(0, 0, aSize.width, aSize.height));
 
   // Copy the source buffer in the _cropRect_ area into a new SourceSurface.
-  RefPtr<DataSourceSurface> result = CropDataSourceSurface(dataSurface, cropRect);
+  RefPtr<DataSourceSurface> result = CropAndCopyDataSourceSurface(dataSurface, cropRect);
 
   if (NS_WARN_IF(!result)) {
     aRv.Throw(NS_ERROR_NOT_AVAILABLE);
     return nullptr;
   }
 
   return result.forget();
 }
@@ -349,44 +414,17 @@ JSObject*
 ImageBitmap::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
 {
   return ImageBitmapBinding::Wrap(aCx, this, aGivenProto);
 }
 
 void
 ImageBitmap::SetPictureRect(const IntRect& aRect, ErrorResult& aRv)
 {
-  gfx::IntRect rect = aRect;
-
-  // fix up negative dimensions
-  if (rect.width < 0) {
-    CheckedInt32 checkedX = CheckedInt32(rect.x) + rect.width;
-
-    if (!checkedX.isValid()) {
-      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
-      return;
-    }
-
-    rect.x = checkedX.value();
-    rect.width = -(rect.width);
-  }
-
-  if (rect.height < 0) {
-    CheckedInt32 checkedY = CheckedInt32(rect.y) + rect.height;
-
-    if (!checkedY.isValid()) {
-      aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
-      return;
-    }
-
-    rect.y = checkedY.value();
-    rect.height = -(rect.height);
-  }
-
-  mPictureRect = rect;
+  mPictureRect = FixUpNegativeDimension(aRect, aRv);
 }
 
 already_AddRefed<SourceSurface>
 ImageBitmap::PrepareForDrawTarget(gfx::DrawTarget* aTarget)
 {
   MOZ_ASSERT(aTarget);
 
   if (!mSurface) {
@@ -438,16 +476,17 @@ ImageBitmap::PrepareForDrawTarget(gfx::D
   }
 
   // Replace our surface with one optimized for the target we're about to draw
   // to, under the assumption it'll likely be drawn again to that target.
   // This call should be a no-op for already-optimized surfaces
   mSurface = target->OptimizeSourceSurface(mSurface);
 
   RefPtr<gfx::SourceSurface> surface(mSurface);
+
   return surface.forget();
 }
 
 /* static */ already_AddRefed<ImageBitmap>
 ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, HTMLImageElement& aImageEl,
                             const Maybe<IntRect>& aCropRect, ErrorResult& aRv)
 {
   // Check if the image element is completely available or not.
@@ -557,17 +596,17 @@ ImageBitmap::CreateInternal(nsIGlobalObj
   if ((aCanvasEl.GetCurrentContextType() == CanvasContextType::WebGL1 ||
        aCanvasEl.GetCurrentContextType() == CanvasContextType::WebGL2) &&
       aCropRect.isSome()) {
     // The _surface_ must be a DataSourceSurface.
     MOZ_ASSERT(surface->GetType() == SurfaceType::DATA,
                "The snapshot SourceSurface from WebGL rendering contest is not \
                DataSourceSurface.");
     RefPtr<DataSourceSurface> dataSurface = surface->GetDataSurface();
-    croppedSurface = CropDataSourceSurface(dataSurface, cropRect);
+    croppedSurface = CropAndCopyDataSourceSurface(dataSurface, cropRect);
     cropRect.MoveTo(0, 0);
   }
   else {
     croppedSurface = surface;
   }
 
   if (NS_WARN_IF(!croppedSurface)) {
     aRv.Throw(NS_ERROR_NOT_AVAILABLE);
@@ -833,17 +872,17 @@ DecodeAndCropBlob(Blob& aBlob, Maybe<Int
 
   if (aCropRect.isSome()) {
     // The blob is just decoded into a RasterImage and not optimized yet, so the
     // _surface_ we get is a DataSourceSurface which wraps the RasterImage's
     // raw buffer.
     MOZ_ASSERT(surface->GetType() == SurfaceType::DATA,
           "The SourceSurface from just decoded Blob is not DataSourceSurface.");
     RefPtr<DataSourceSurface> dataSurface = surface->GetDataSurface();
-    croppedSurface = CropDataSourceSurface(dataSurface, aCropRect.ref());
+    croppedSurface = CropAndCopyDataSourceSurface(dataSurface, aCropRect.ref());
     aCropRect->MoveTo(0, 0);
   }
 
   if (NS_WARN_IF(!croppedSurface)) {
     aRv.Throw(NS_ERROR_NOT_AVAILABLE);
     return nullptr;
   }