fix: add missing buffer size check in nativeImage (electron#17569)

trop[bot] · MarshallOfSound · commit a4bea62b98ba · 2019-03-27T11:35:01.000-07:00
diff --git a/atom/common/api/atom_api_native_image.cc b/atom/common/api/atom_api_native_image.cc
@@ -79,49 +79,71 @@ float GetScaleFactorFromOptions(mate::Arguments* args) {
   return scale_factor;
 }
 
-bool AddImageSkiaRep(gfx::ImageSkia* image,
-                     const unsigned char* data,
-                     size_t size,
-                     int width,
-                     int height,
-                     double scale_factor) {
-  auto decoded = std::make_unique<SkBitmap>();
+bool AddImageSkiaRepFromPNG(gfx::ImageSkia* image,
+                            const unsigned char* data,
+                            size_t size,
+                            double scale_factor) {
+  SkBitmap bitmap;
+  if (!gfx::PNGCodec::Decode(data, size, &bitmap))
+    return false;
+
+  image->AddRepresentation(gfx::ImageSkiaRep(bitmap, scale_factor));
+  return true;
+}
+
+bool AddImageSkiaRepFromJPEG(gfx::ImageSkia* image,
+                             const unsigned char* data,
+                             size_t size,
+                             double scale_factor) {
+  auto bitmap = gfx::JPEGCodec::Decode(data, size);
+  if (!bitmap)
+    return false;
+
+  // `JPEGCodec::Decode()` doesn't tell `SkBitmap` instance it creates
+  // that all of its pixels are opaque, that's why the bitmap gets
+  // an alpha type `kPremul_SkAlphaType` instead of `kOpaque_SkAlphaType`.
+  // Let's fix it here.
+  // TODO(alexeykuzmin): This workaround should be removed
+  // when the `JPEGCodec::Decode()` code is fixed.
+  // See https://github.com/electron/electron/issues/11294.
+  bitmap->setAlphaType(SkAlphaType::kOpaque_SkAlphaType);
+
+  image->AddRepresentation(gfx::ImageSkiaRep(*bitmap, scale_factor));
+  return true;
+}
 
+bool AddImageSkiaRepFromBuffer(gfx::ImageSkia* image,
+                               const unsigned char* data,
+                               size_t size,
+                               int width,
+                               int height,
+                               double scale_factor) {
   // Try PNG first.
-  if (!gfx::PNGCodec::Decode(data, size, decoded.get())) {
-    // Try JPEG.
-    decoded = gfx::JPEGCodec::Decode(data, size);
-    if (decoded) {
-      // `JPEGCodec::Decode()` doesn't tell `SkBitmap` instance it creates
-      // that all of its pixels are opaque, that's why the bitmap gets
-      // an alpha type `kPremul_SkAlphaType` instead of `kOpaque_SkAlphaType`.
-      // Let's fix it here.
-      // TODO(alexeykuzmin): This workaround should be removed
-      // when the `JPEGCodec::Decode()` code is fixed.
-      // See https://github.com/electron/electron/issues/11294.
-      decoded->setAlphaType(SkAlphaType::kOpaque_SkAlphaType);
-    }
-  }
+  if (AddImageSkiaRepFromPNG(image, data, size, scale_factor))
+    return true;
 
-  if (!decoded) {
-    // Try Bitmap
-    if (width > 0 && height > 0) {
-      decoded.reset(new SkBitmap);
-      decoded->allocN32Pixels(width, height, false);
-      decoded->setPixels(
-          const_cast<void*>(reinterpret_cast<const void*>(data)));
-    } else {
-      return false;
-    }
-  }
+  // Try JPEG second.
+  if (AddImageSkiaRepFromJPEG(image, data, size, scale_factor))
+    return true;
 
-  image->AddRepresentation(gfx::ImageSkiaRep(*decoded, scale_factor));
+  if (width == 0 || height == 0)
+    return false;
+
+  auto info = SkImageInfo::MakeN32(width, height, kPremul_SkAlphaType);
+  if (size < info.computeMinByteSize())
+    return false;
+
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(width, height, false);
+  bitmap.setPixels(const_cast<void*>(reinterpret_cast<const void*>(data)));
+
+  image->AddRepresentation(gfx::ImageSkiaRep(bitmap, scale_factor));
   return true;
 }
 
-bool AddImageSkiaRep(gfx::ImageSkia* image,
-                     const base::FilePath& path,
-                     double scale_factor) {
+bool AddImageSkiaRepFromPath(gfx::ImageSkia* image,
+                             const base::FilePath& path,
+                             double scale_factor) {
   std::string file_contents;
   {
     base::ThreadRestrictions::ScopedAllowIO allow_io;
@@ -132,7 +154,8 @@ bool AddImageSkiaRep(gfx::ImageSkia* image,
   const unsigned char* data =
       reinterpret_cast<const unsigned char*>(file_contents.data());
   size_t size = file_contents.size();
-  return AddImageSkiaRep(image, data, size, 0, 0, scale_factor);
+
+  return AddImageSkiaRepFromBuffer(image, data, size, 0, 0, scale_factor);
 }
 
 bool PopulateImageSkiaRepsFromPath(gfx::ImageSkia* image,
@@ -141,12 +164,12 @@ bool PopulateImageSkiaRepsFromPath(gfx::ImageSkia* image,
   std::string filename(path.BaseName().RemoveExtension().AsUTF8Unsafe());
   if (base::MatchPattern(filename, "*@*x"))
     // Don't search for other representations if the DPI has been specified.
-    return AddImageSkiaRep(image, path, GetScaleFactorFromPath(path));
+    return AddImageSkiaRepFromPath(image, path, GetScaleFactorFromPath(path));
   else
-    succeed |= AddImageSkiaRep(image, path, 1.0f);
+    succeed |= AddImageSkiaRepFromPath(image, path, 1.0f);
 
   for (const ScaleFactorPair& pair : kScaleFactorPairs)
-    succeed |= AddImageSkiaRep(
+    succeed |= AddImageSkiaRepFromPath(
         image, path.InsertBeforeExtensionASCII(pair.name), pair.scale);
   return succeed;
 }
@@ -422,19 +445,20 @@ void NativeImage::AddRepresentation(const mate::Dictionary& options) {
   v8::Local<v8::Value> buffer;
   GURL url;
   if (options.Get("buffer", &buffer) && node::Buffer::HasInstance(buffer)) {
-    AddImageSkiaRep(
-        &image_skia,
-        reinterpret_cast<unsigned char*>(node::Buffer::Data(buffer)),
-        node::Buffer::Length(buffer), width, height, scale_factor);
-    skia_rep_added = true;
+    auto* data = reinterpret_cast<unsigned char*>(node::Buffer::Data(buffer));
+    auto size = node::Buffer::Length(buffer);
+    skia_rep_added = AddImageSkiaRepFromBuffer(&image_skia, data, size, width,
+                                               height, scale_factor);
   } else if (options.Get("dataURL", &url)) {
     std::string mime_type, charset, data;
     if (net::DataURL::Parse(url, &mime_type, &charset, &data)) {
-      if (mime_type == "image/png" || mime_type == "image/jpeg") {
-        AddImageSkiaRep(&image_skia,
-                        reinterpret_cast<const unsigned char*>(data.c_str()),
-                        data.size(), width, height, scale_factor);
-        skia_rep_added = true;
+      auto* data_ptr = reinterpret_cast<const unsigned char*>(data.c_str());
+      if (mime_type == "image/png") {
+        skia_rep_added = AddImageSkiaRepFromPNG(&image_skia, data_ptr,
+                                                data.size(), scale_factor);
+      } else if (mime_type == "image/jpeg") {
+        skia_rep_added = AddImageSkiaRepFromJPEG(&image_skia, data_ptr,
+                                                 data.size(), scale_factor);
       }
     }
   }
@@ -525,9 +549,9 @@ mate::Handle<NativeImage> NativeImage::CreateFromBuffer(
   }
 
   gfx::ImageSkia image_skia;
-  AddImageSkiaRep(&image_skia,
-                  reinterpret_cast<unsigned char*>(node::Buffer::Data(buffer)),
-                  node::Buffer::Length(buffer), width, height, scale_factor);
+  AddImageSkiaRepFromBuffer(
+      &image_skia, reinterpret_cast<unsigned char*>(node::Buffer::Data(buffer)),
+      node::Buffer::Length(buffer), width, height, scale_factor);
   return Create(args->isolate(), gfx::Image(image_skia));
 }
 
diff --git a/spec/api-native-image-spec.js b/spec/api-native-image-spec.js
@@ -130,6 +130,11 @@ describe('nativeImage module', () => {
       expect(nativeImage.createFromBuffer(Buffer.from([])).isEmpty())
     })
 
+    it('returns an empty image when the buffer is too small', () => {
+      const image = nativeImage.createFromBuffer(Buffer.from([1, 2, 3, 4]), { width: 100, height: 100 })
+      expect(image.isEmpty()).to.be.true()
+    })
+
     it('returns an image created from the given buffer', () => {
       const imageA = nativeImage.createFromPath(path.join(__dirname, 'fixtures', 'assets', 'logo.png'))
 
@@ -450,6 +455,18 @@ describe('nativeImage module', () => {
   })
 
   describe('addRepresentation()', () => {
+    it('does not add representation when the buffer is too small', () => {
+      const image = nativeImage.createEmpty()
+
+      image.addRepresentation({
+        buffer: Buffer.from([1, 2, 3, 4]),
+        width: 100,
+        height: 100
+      })
+
+      expect(image.isEmpty()).to.be.true()
+    })
+
     it('supports adding a buffer representation for a scale factor', () => {
       const image = nativeImage.createEmpty()
 
