Introduce skia.mojom.UnsafeBitmap for more efficient bitmap transfer

This type is binary compatible with skia.mojom.Bitmap and only serves
to specify a difference in the generated bindings. Instead of copying
the pixels out of shared memory into a heap allocated buffer retained
by the SkBitmap, the bindings will allow the SkBitmap to simply retain
the given shared memory buffer. This avoids a potentially large copy.

This change leverages SkBitmap's support for having its pixel backing
managed externally via SkPixelRef. The BigBuffer received over IPC is
stored in a new SkPixelRef subclass. The outcome of this is that if
the BigBuffer was using shared memory then the SkBitmap will be able
to reference that memory directly, thus avoiding a copy of the pixels.

Finally, the Lacros code for screen capture is updated to use this new
type to eliminate a large buffer copy.

Bug: 1094460
Change-Id: I9249ad40ced16c8cbe42bbbe5d11d9820cc75d7f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2450838Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Commit-Queue: Darin Fisher <darin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815217}

chromeos/crosapi/mojom/screen_manager.mojom

@@ -28,7 +28,8 @@ interface SnapshotCapturer {
   // Captures a bitmap snapshot of the specified screen or window. If |success|
   // is false, that may indicate that the specified source no longer exists.
   [Sync]
-  TakeSnapshot@1(uint64 id) => (bool success, skia.mojom.Bitmap? snapshot);
+  TakeSnapshot@1(uint64 id) => (bool success,
+                                skia.mojom.UnsafeBitmap? snapshot);
 };
 
 // This interface is implemented by ash-chrome. It allows lacros-chrome to query

skia/public/mojom/BUILD.gn

@@ -57,6 +57,11 @@ mojom("mojom") {
           cpp = "::SkBitmap"
           nullable_is_same_type = true
         },
+        {
+          mojom = "skia.mojom.UnsafeBitmap"
+          cpp = "::SkBitmap"
+          nullable_is_same_type = true
+        },
         {
           mojom = "skia.mojom.InlineBitmap"
           cpp = "::SkBitmap"

skia/public/mojom/bitmap.mojom

@@ -15,6 +15,23 @@ struct Bitmap {
   mojo_base.mojom.BigBuffer pixel_data;
 };
 
+// Similar to above, but the generated bindings avoid copying pixel data on the
+// receiving side of an IPC message. That can be a valuable optimization for
+// large bitmaps. However, this is unsafe for some applications as it leaves
+// open the possibility for the sender to continue to modify the pixel data,
+// which could lead to TOCTOU issues. Use this type *only* when the sender is
+// trusted.
+//
+// NOTE: It is important that the fields of this struct exactly match the
+// fields of the Bitmap struct. This enables stable interfaces to freely
+// migrate between these two types in a compatible fashion.
+[Stable]
+struct UnsafeBitmap {
+  ImageInfo image_info;
+  uint64 row_bytes;
+  mojo_base.mojom.BigBuffer pixel_data;
+};
+
 // NOTE: This should only be used when an SkBitmap MUST be serialized as raw
 // bytes (i.e. it's not OK for shared memory to be used, as above).
 struct InlineBitmap {

skia/public/mojom/bitmap_skbitmap_mojom_traits.cc

@@ -4,6 +4,8 @@
 
 #include "skia/public/mojom/bitmap_skbitmap_mojom_traits.h"
 
+#include "third_party/skia/include/core/SkPixelRef.h"
+
 namespace mojo {
 namespace {
 
@@ -12,6 +14,21 @@ namespace {
 constexpr int kMaxWidth = 32 * 1024;
 constexpr int kMaxHeight = 32 * 1024;
 
+// A custom SkPixelRef subclass to wrap a BigBuffer storing the pixel data.
+class BigBufferPixelRef final : public SkPixelRef {
+ public:
+  BigBufferPixelRef(mojo_base::BigBuffer buffer,
+                    int width,
+                    int height,
+                    int row_bytes)
+      : SkPixelRef(width, height, buffer.data(), row_bytes),
+        buffer_(std::move(buffer)) {}
+  ~BigBufferPixelRef() override = default;
+
+ private:
+  mojo_base::BigBuffer buffer_;
+};
+
 }  // namespace
 
 // static
@@ -77,12 +94,86 @@ bool StructTraits<skia::mojom::BitmapDataView, SkBitmap>::Read(
     return false;
   }
 
+  // Implementation note: This copy is important from a security perspective as
+  // it provides the recipient of the SkBitmap with a stable copy of the data.
+  // The sender could otherwise continue modifying the shared memory buffer
+  // underlying the BigBuffer instance.
   std::copy(pixel_data_bytes.begin(), pixel_data_bytes.end(),
             static_cast<uint8_t*>(b->getPixels()));
   b->notifyPixelsChanged();
   return true;
 }
 
+// static
+bool StructTraits<skia::mojom::UnsafeBitmapDataView, SkBitmap>::IsNull(
+    const SkBitmap& b) {
+  return b.isNull();
+}
+
+// static
+void StructTraits<skia::mojom::UnsafeBitmapDataView, SkBitmap>::SetToNull(
+    SkBitmap* b) {
+  b->reset();
+}
+
+// static
+const SkImageInfo& StructTraits<skia::mojom::UnsafeBitmapDataView,
+                                SkBitmap>::image_info(const SkBitmap& b) {
+  return b.info();
+}
+
+// static
+uint64_t StructTraits<skia::mojom::UnsafeBitmapDataView, SkBitmap>::row_bytes(
+    const SkBitmap& b) {
+  return b.rowBytes();
+}
+
+// static
+mojo_base::BigBufferView StructTraits<skia::mojom::UnsafeBitmapDataView,
+                                      SkBitmap>::pixel_data(const SkBitmap& b) {
+  return mojo_base::BigBufferView(base::make_span(
+      static_cast<uint8_t*>(b.getPixels()), b.computeByteSize()));
+}
+
+// static
+bool StructTraits<skia::mojom::UnsafeBitmapDataView, SkBitmap>::Read(
+    skia::mojom::UnsafeBitmapDataView data,
+    SkBitmap* b) {
+  SkImageInfo image_info;
+  if (!data.ReadImageInfo(&image_info))
+    return false;
+
+  // Ensure width and height are reasonable.
+  if (image_info.width() > kMaxWidth || image_info.height() > kMaxHeight)
+    return false;
+
+  *b = SkBitmap();
+
+  // If the image is empty, return success after setting the image info.
+  if (image_info.width() == 0 || image_info.height() == 0)
+    return b->tryAllocPixels(image_info, data.row_bytes());
+
+  // Otherwise, set a custom PixelRef to retain the BigBuffer. This avoids
+  // making another copy of the pixel data.
+
+  mojo_base::BigBufferView pixel_data_view;
+  if (!data.ReadPixelData(&pixel_data_view))
+    return false;
+
+  if (!b->setInfo(image_info, data.row_bytes()))
+    return false;
+
+  // Allow the resultant SkBitmap to refer to the given BigBuffer. Note, the
+  // sender could continue modifying the pixels of the buffer, which could be a
+  // security concern for some applications. The trade-off is performance.
+  b->setPixelRef(
+      sk_make_sp<BigBufferPixelRef>(
+          mojo_base::BigBufferView::ToBigBuffer(std::move(pixel_data_view)),
+          image_info.width(), image_info.height(), data.row_bytes()),
+      0, 0);
+  return true;
+}
+
 // static
 bool StructTraits<skia::mojom::InlineBitmapDataView, SkBitmap>::IsNull(
     const SkBitmap& b) {

skia/public/mojom/bitmap_skbitmap_mojom_traits.h

@@ -28,6 +28,17 @@ struct COMPONENT_EXPORT(SKIA_SHARED_TRAITS)
   static bool Read(skia::mojom::BitmapDataView data, SkBitmap* b);
 };
 
+template <>
+struct COMPONENT_EXPORT(SKIA_SHARED_TRAITS)
+    StructTraits<skia::mojom::UnsafeBitmapDataView, SkBitmap> {
+  static bool IsNull(const SkBitmap& b);
+  static void SetToNull(SkBitmap* b);
+  static const SkImageInfo& image_info(const SkBitmap& b);
+  static uint64_t row_bytes(const SkBitmap& b);
+  static mojo_base::BigBufferView pixel_data(const SkBitmap& b);
+  static bool Read(skia::mojom::UnsafeBitmapDataView data, SkBitmap* b);
+};
+
 template <>
 struct COMPONENT_EXPORT(SKIA_SHARED_TRAITS)
     StructTraits<skia::mojom::InlineBitmapDataView, SkBitmap> {