From b261e1ba70cebb48b58560978af8a5978befe37e Mon Sep 17 00:00:00 2001
From: Kai Ninomiya <[email protected]>
Date: Mon, 17 Aug 2026 17:49:52 -0700
Subject: [PATCH] Fix incorrectly copying row padding in command buffer clients

VideoFrame::copyTo() is supposed to copy only the pixels and not the
padding bytes between rows. Doing so requires one memcpy per row instead
of one big memcpy.

- RasterImplementation::ReadbackImagePixelsINTERNAL and
  RasterImplementation::OnAsyncARGBReadbackDone:
  accessible via VideoFrame::copyTo.
  - There are many other codepaths reachable from VideoFrame::copyTo().
    The others seem to be correct.
- RasterImplementation::AsyncYUVReadbackRequest::CopyYUVPlane and
  GLES2Implementation::ReadbackARGBImagePixelsINTERNAL:
  also seemed to do this wrong, but I have not looked into exactly how
  they're used, so I'm not certain they're important or whether they
  have a security impact. I'm only guessing they are important based on
  the similar comments that said they write into JS-visible memory
  (which is how I stumbled on them).

Tests:
- raster_implementation_unittest tests generated using Gemini, but
  manually verified that each of the four tests fails when the fix it
  targets is reverted (e.g. see how the first three were tested in
  patchset 4: https://crrev.com/c/8255469/4).
  - Manual drive-by fix some of the initialization sites of
    MockTransferBuffer::ExpectedMemoryInfo, so that the spanified form
    can be used instead of raw pointers in the new test.
- content/test/data/gpu/webcodecs/copyTo.html test also generated using
  Gemini, but manually verified that it hits the bug in
  RasterImplementation::ReadbackImagePixelsINTERNAL. It doesn't seem to
  hit any of the others on my device (Pixel 3).
- WPT test for copyTo() on VideoFrames that are CPU-backed (which was
  already fine) and texture-backed (which is what this fixes).
  Unfortunately this test does not seem to catch the issue on CQ, but
  the texture-backed one does at least catch it locally on my Mac.

Fixed: 543707066
Change-Id: Iea2e75cdb75175f250ab5472a22c2f46819e6e84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8255469
Reviewed-by: Eugene Zemtsov <[email protected]>
Commit-Queue: Kai Ninomiya <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1680958}
---

diff --git a/content/test/data/gpu/webcodecs/copyTo.html b/content/test/data/gpu/webcodecs/copyTo.html
index efef2057..5bca638 100644
--- a/content/test/data/gpu/webcodecs/copyTo.html
+++ b/content/test/data/gpu/webcodecs/copyTo.html
@@ -198,6 +198,78 @@
       worker.terminate();
     }
 
+    // Test that padding bytes (stride/offset padding) are not overwritten.
+    {
+      const PAD = 0xAA;
+      const options = {
+        layout: [
+          {offset: 5, stride: frame.displayWidth * 4 + 16}, // plenty of padding
+        ],
+      };
+      if (frame.format === 'I420' || frame.format === 'I420A') {
+        options.layout = [
+          {offset: 5, stride: frame.displayWidth + 16},
+          {offset: (frame.displayWidth + 16) * frame.displayHeight + 10, stride: frame.displayWidth / 2 + 16},
+          {offset: (frame.displayWidth + 16) * frame.displayHeight + (frame.displayWidth / 2 + 16) * (frame.displayHeight / 2) + 20, stride: frame.displayWidth / 2 + 16},
+        ];
+        if (frame.format === 'I420A') {
+          options.layout.push({
+            offset: (frame.displayWidth + 16) * frame.displayHeight + (frame.displayWidth / 2 + 16) * (frame.displayHeight / 2) * 2 + 30,
+            stride: frame.displayWidth + 16
+          });
+        }
+      } else if (frame.format === 'NV12') {
+        options.layout = [
+          {offset: 5, stride: frame.displayWidth + 16},
+          {offset: (frame.displayWidth + 16) * frame.displayHeight + 10, stride: frame.displayWidth + 16},
+        ];
+      }
+
+      let size_with_padding = frame.allocationSize(options);
+      let buf = new ArrayBuffer(size_with_padding);
+      let view = new Uint8Array(buf);
+      view.fill(PAD);
+
+      let layout = await frame.copyTo(buf, options);
+
+      // Verify that the layout matches what we specified
+      for (let plane = 0; plane < layout.length; plane++) {
+        TEST.assert(layout[plane].offset === options.layout[plane].offset, "offset mismatch");
+        TEST.assert(layout[plane].stride === options.layout[plane].stride, "stride mismatch");
+      }
+
+      // We want to verify that the bytes between rows (the padding bytes) are untouched (still PAD).
+      for (let plane = 0; plane < layout.length; plane++) {
+        let plane_offset = layout[plane].offset;
+        let stride = layout[plane].stride;
+        let plane_height = frame.displayHeight;
+        let plane_width = frame.displayWidth;
+        if (plane > 0 && (frame.format === 'I420' || frame.format === 'I420A' || frame.format === 'NV12')) {
+          plane_height /= 2;
+          if (frame.format === 'I420' || frame.format === 'I420A') {
+            plane_width /= 2;
+          }
+        }
+        let bytes_per_pixel = 1;
+        if (frame.format === 'RGBA' || frame.format === 'RGBX' || frame.format === 'BGRA' || frame.format === 'BGRX') {
+          bytes_per_pixel = 4;
+        } else if (frame.format === 'NV12' && plane === 1) {
+          bytes_per_pixel = 2; // UV interleaved
+        }
+
+        let row_bytes = plane_width * bytes_per_pixel;
+        for (let y = 0; y < plane_height; ++y) {
+          let row_start = plane_offset + y * stride;
+          for (let x = row_bytes; x < stride; ++x) {
+            let pad_idx = row_start + x;
+            if (pad_idx < size_with_padding) {
+              TEST.assert(view[pad_idx] === PAD, `Padding byte overwritten at index ${pad_idx} for plane ${plane}, row ${y}, x ${x}. Expected ${PAD}, got ${view[pad_idx]}`);
+            }
+          }
+        }
+      }
+    }
+
     // Validate pixels
     if (!arg.validate_camera_frames && source_type == 'camera') {
       TEST.log("Skip copyTo result validation");
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index c2ce32c9..fbfc14a 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -59,6 +59,7 @@
 #include "gpu/command_buffer/common/swap_buffers_complete_params.h"
 #include "gpu/command_buffer/common/sync_token.h"
 #include "third_party/skia/include/core/SkAlphaType.h"
+#include "third_party/skia/include/core/SkColorType.h"
 #include "third_party/skia/include/gpu/ganesh/GrTypes.h"
 #include "ui/gfx/color_space.h"
 #include "ui/gfx/geometry/rect.h"
@@ -4649,12 +4650,15 @@
   if (!*readback_result) {
     return GL_FALSE;
   }
-  // We need to use `RelaxedAtomicWriteMemcpy` because we might be writing into
-  // memory observed by JS at the same time.
   auto dst = UNSAFE_TODO(base::span(static_cast<uint8_t*>(pixels), dst_size));
   auto src = UNSAFE_TODO(
       base::span(static_cast<uint8_t*>(shm_address) + pixels_offset, dst_size));
-  base::subtle::RelaxedAtomicWriteMemcpy(dst, src);
+  size_t min_row_bytes =
+      dst_width *
+      SkColorTypeBytesPerPixel(static_cast<SkColorType>(dst_sk_color_type));
+  RelaxedAtomicWriteMemcpyImageRowsSkippingPadding(
+      /*dst=*/dst, /*src=*/src, /*row_bytes=*/min_row_bytes,
+      /*height=*/dst_height, /*stride=*/dst_row_bytes);
   return GL_TRUE;
 }
 
diff --git a/gpu/command_buffer/client/gles2_implementation_unittest.cc b/gpu/command_buffer/client/gles2_implementation_unittest.cc
index 51380ad6c..0de18a57 100644
--- a/gpu/command_buffer/client/gles2_implementation_unittest.cc
+++ b/gpu/command_buffer/client/gles2_implementation_unittest.cc
@@ -17,6 +17,7 @@
 #include <array>
 #include <memory>
 
+#include "base/bits.h"
 #include "base/compiler_specific.h"
 #include "base/containers/heap_array.h"
 #include "base/containers/span.h"
@@ -403,7 +404,8 @@
     ExpectedMemoryInfo mem;
 
     // Temporarily allocate memory and expect that memory block to be reused.
-    mem.ptr = gl_->mapped_memory_->Alloc(size, &mem.id, &mem.offset).data();
+    mem.span = gl_->mapped_memory_->Alloc(size, &mem.id, &mem.offset);
+    mem.ptr = mem.span.data();
     gl_->mapped_memory_->Free(mem.ptr);
 
     return mem;
@@ -3864,6 +3866,75 @@
                                  "but written again"));
 }
 
+TEST_F(GLES2ImplementationTest, ReadbackARGBImagePixelsINTERNALPadding) {
+  gpu::Mailbox mailbox = gpu::Mailbox::Generate();
+
+  GLuint dst_width = 2;
+  GLuint dst_height = 2;
+  GLuint dst_sk_color_type = 4;  // kRGBA_8888_SkColorType
+  GLuint dst_sk_alpha_type = 1;  // kPremul_SkAlphaType
+  GLuint dst_row_bytes =
+      12;  // 2 pixels * 4 bytes/pixel = 8 bytes. Row padding = 4 bytes.
+  GLuint dst_size = dst_height * dst_row_bytes;
+
+  GLuint color_space_offset = base::bits::AlignUp(
+      sizeof(cmds::ReadbackARGBImagePixelsINTERNAL::Result), sizeof(uint64_t));

diff --git a/content/test/data/gpu/webcodecs/copyTo.html b/content/test/data/gpu/webcodecs/copyTo.html
index efef2057..5bca638 100644
--- a/content/test/data/gpu/webcodecs/copyTo.html
+++ b/content/test/data/gpu/webcodecs/copyTo.html
@@ -198,6 +198,78 @@
       worker.terminate();
     }
 
+    // Test that padding bytes (stride/offset padding) are not overwritten.
+    {
+      const PAD = 0xAA;
+      const options = {
+        layout: [
+          {offset: 5, stride: frame.displayWidth * 4 + 16}, // plenty of padding
+        ],
+      };
+      if (frame.format === 'I420' || frame.format === 'I420A') {
+        options.layout = [
+          {offset: 5, stride: frame.displayWidth + 16},
+          {offset: (frame.displayWidth + 16) * frame.displayHeight + 10, stride: frame.displayWidth / 2 + 16},
+          {offset: (frame.displayWidth + 16) * frame.displayHeight + (frame.displayWidth / 2 + 16) * (frame.displayHeight / 2) + 20, stride: frame.displayWidth / 2 + 16},
+        ];
+        if (frame.format === 'I420A') {
+          options.layout.push({
+            offset: (frame.displayWidth + 16) * frame.displayHeight + (frame.displayWidth / 2 + 16) * (frame.displayHeight / 2) * 2 + 30,
+            stride: frame.displayWidth + 16
+          });
+        }
+      } else if (frame.format === 'NV12') {
+        options.layout = [
+          {offset: 5, stride: frame.displayWidth + 16},
+          {offset: (frame.displayWidth + 16) * frame.displayHeight + 10, stride: frame.displayWidth + 16},
+        ];
+      }
+
+      let size_with_padding = frame.allocationSize(options);
+      let buf = new ArrayBuffer(size_with_padding);
+      let view = new Uint8Array(buf);
+      view.fill(PAD);
+
+      let layout = await frame.copyTo(buf, options);
+
+      // Verify that the layout matches what we specified
+      for (let plane = 0; plane < layout.length; plane++) {
+        TEST.assert(layout[plane].offset === options.layout[plane].offset, "offset mismatch");
+        TEST.assert(layout[plane].stride === options.layout[plane].stride, "stride mismatch");
+      }
+
+      // We want to verify that the bytes between rows (the padding bytes) are untouched (still PAD).
+      for (let plane = 0; plane < layout.length; plane++) {
+        let plane_offset = layout[plane].offset;
+        let stride = layout[plane].stride;
+        let plane_height = frame.displayHeight;
+        let plane_width = frame.displayWidth;
+        if (plane > 0 && (frame.format === 'I420' || frame.format === 'I420A' || frame.format === 'NV12')) {
+          plane_height /= 2;
+          if (frame.format === 'I420' || frame.format === 'I420A') {
+            plane_width /= 2;
+          }
+        }
+        let bytes_per_pixel = 1;
+        if (frame.format === 'RGBA' || frame.format === 'RGBX' || frame.format === 'BGRA' || frame.format === 'BGRX') {
+          bytes_per_pixel = 4;
+        } else if (frame.format === 'NV12' && plane === 1) {
+          bytes_per_pixel = 2; // UV interleaved
+        }
+
+        let row_bytes = plane_width * bytes_per_pixel;
+        for (let y = 0; y < plane_height; ++y) {
+          let row_start = plane_offset + y * stride;
+          for (let x = row_bytes; x < stride; ++x) {
+            let pad_idx = row_start + x;
+            if (pad_idx < size_with_padding) {
+              TEST.assert(view[pad_idx] === PAD, `Padding byte overwritten at index ${pad_idx} for plane ${plane}, row ${y}, x ${x}. Expected ${PAD}, got ${view[pad_idx]}`);
+            }
+          }
+        }
+      }
+    }
+
     // Validate pixels
     if (!arg.validate_camera_frames && source_type == 'camera') {
       TEST.log("Skip copyTo result validation");
diff --git a/gpu/command_buffer/client/gles2_implementation_unittest.cc b/gpu/command_buffer/client/gles2_implementation_unittest.cc
index 51380ad6c..0de18a57 100644
--- a/gpu/command_buffer/client/gles2_implementation_unittest.cc
+++ b/gpu/command_buffer/client/gles2_implementation_unittest.cc
@@ -17,6 +17,7 @@
 #include <array>
 #include <memory>
 
+#include "base/bits.h"
 #include "base/compiler_specific.h"
 #include "base/containers/heap_array.h"
 #include "base/containers/span.h"
@@ -403,7 +404,8 @@
     ExpectedMemoryInfo mem;
 
     // Temporarily allocate memory and expect that memory block to be reused.
-    mem.ptr = gl_->mapped_memory_->Alloc(size, &mem.id, &mem.offset).data();
+    mem.span = gl_->mapped_memory_->Alloc(size, &mem.id, &mem.offset);
+    mem.ptr = mem.span.data();
     gl_->mapped_memory_->Free(mem.ptr);
 
     return mem;
@@ -3864,6 +3866,75 @@
                                  "but written again"));
 }
 
+TEST_F(GLES2ImplementationTest, ReadbackARGBImagePixelsINTERNALPadding) {
+  gpu::Mailbox mailbox = gpu::Mailbox::Generate();
+
+  GLuint dst_width = 2;
+  GLuint dst_height = 2;
+  GLuint dst_sk_color_type = 4;  // kRGBA_8888_SkColorType
+  GLuint dst_sk_alpha_type = 1;  // kPremul_SkAlphaType
+  GLuint dst_row_bytes =
+      12;  // 2 pixels * 4 bytes/pixel = 8 bytes. Row padding = 4 bytes.
+  GLuint dst_size = dst_height * dst_row_bytes;
+
+  GLuint color_space_offset = base::bits::AlignUp(
+      sizeof(cmds::ReadbackARGBImagePixelsINTERNAL::Result), sizeof(uint64_t));
+  GLuint mailbox_offset = color_space_offset;
+  GLuint pixels_offset = base::bits::AlignUp(
+      mailbox_offset + sizeof(gpu::Mailbox), sizeof(uint64_t));
+
+  GLuint total_size =
+      pixels_offset +
+      base::bits::AlignUp(dst_size, static_cast<GLuint>(sizeof(uint64_t)));
+
+  ExpectedMemoryInfo mem = GetExpectedMappedMemory(total_size);
+
+  std::vector<uint8_t> dst_pixels(dst_size, 0xAA);
+
+  EXPECT_CALL(*command_buffer(), OnFlush())
+      .WillOnce([mem, pixels_offset, dst_size]() {
+        // Write 1 to readback_result (at the beginning of shm).
+        auto* result =
+            reinterpret_cast<cmds::ReadbackARGBImagePixelsINTERNAL::Result*>(
+                mem.ptr);
+        *result = 1;
+
+        // Write test data to the pixel portion of the shared memory.
+        auto src_pixels = mem.span.subspan(pixels_offset, dst_size);
+        // Fill src_pixels with distinct values, e.g. 1 to dst_size
+        for (size_t i = 0; i < dst_size; ++i) {
+          src_pixels[i] = static_cast<uint8_t>(i + 1);
+        }
+      })
+      .RetiresOnSaturation();
+
+  GLboolean success = gl_->ReadbackARGBImagePixelsINTERNAL(
+      mailbox.name, /*dst_color_space=*/nullptr,
+      /*dst_color_space_size=*/0, dst_size, dst_width, dst_height,
+      dst_sk_color_type, dst_sk_alpha_type, dst_row_bytes, /*src_x=*/0,
+      /*src_y=*/0, /*plane_index=*/0, dst_pixels.data());
+
+  EXPECT_TRUE(success);
+
+  // Expected output:
+  // Row 1 (pixels: 0 to 7) copied from src_pixels (0 to 7): 1, 2, 3, 4, 5, 6,
+  // 7, 8. Row 1 (padding: 8 to 11) untouched: 0xAA, 0xAA, 0xAA, 0xAA. Row 2
+  // (pixels: 12 to 19) copied from src_pixels (12 to 19): 13, 14, 15, 16, 17,
+  // 18, 19, 20. Row 2 (padding: 20 to 23) untouched: 0xAA, 0xAA, 0xAA, 0xAA.
+
+  std::vector<uint8_t> expected_pixels(dst_size, 0xAA);
+  size_t min_row_bytes = 8;  // 2 pixels * 4 bytes/pixel = 8.
+  for (size_t y = 0; y < dst_height; ++y) {
+    for (size_t x = 0; x < min_row_bytes; ++x) {
+      size_t dst_idx = y * dst_row_bytes + x;
+      size_t src_idx = y * dst_row_bytes + x;
+      expected_pixels[dst_idx] = static_cast<uint8_t>(src_idx + 1);
+    }
+  }
+
+  EXPECT_EQ(dst_pixels, expected_pixels);
+}
+
 #include "gpu/command_buffer/client/gles2_implementation_unittest_autogen.h"
 
 }  // namespace gles2
diff --git a/gpu/command_buffer/client/raster_implementation_unittest.cc b/gpu/command_buffer/client/raster_implementation_unittest.cc
index e8b306e..d9fd57a 100644
--- a/gpu/command_buffer/client/raster_implementation_unittest.cc
+++ b/gpu/command_buffer/client/raster_implementation_unittest.cc
@@ -16,6 +16,7 @@
 #include <array>
 #include <memory>
 
+#include "base/bits.h"
 #include "base/compiler_specific.h"
 #include "base/containers/heap_array.h"
 #include "base/containers/span.h"
@@ -254,7 +255,8 @@
     ExpectedMemoryInfo mem;
 
     // Temporarily allocate memory and expect that memory block to be reused.
-    mem.ptr = gl_->mapped_memory_->Alloc(size, &mem.id, &mem.offset).data();
+    mem.span = gl_->mapped_memory_->Alloc(size, &mem.id, &mem.offset);
+    mem.ptr = mem.span.data();
     gl_->mapped_memory_->Free(mem.ptr);
 
     return mem;
@@ -275,6 +277,19 @@
     return gl_->GetBucketContents(bucket_id, data);
   }
 
+  bool ReadbackImagePixelsINTERNAL(const gpu::Mailbox& source_mailbox,
+                                   const SkImageInfo& dst_info,
+                                   GLuint dst_row_bytes,
+                                   int src_x,
+                                   int src_y,
+                                   int plane_index,
+                                   base::OnceCallback<void(bool)> readback_done,
+                                   void* dst_pixels) {
+    return gl_->ReadbackImagePixelsINTERNAL(
+        source_mailbox, dst_info, dst_row_bytes, src_x, src_y, plane_index,
+        std::move(readback_done), dst_pixels);
+  }
+
   static SharedMemoryLimits SharedMemoryLimitsForTesting() {
     SharedMemoryLimits limits;
     limits.command_buffer_size = kCommandBufferSizeBytes;
@@ -879,6 +894,275 @@
   EXPECT_TRUE(NoCommandsWritten());
 }
 
+// https://crbug.com/543707066
+TEST_F(RasterImplementationTest, ReadbackImagePixelsSyncPadding) {
+  gpu::Mailbox mailbox = gpu::Mailbox::Generate();
+  SkImageInfo dst_info = SkImageInfo::MakeN32Premul(2, 2);
+  GLuint dst_row_bytes =
+      12;  // 2 pixels * 4 bytes/pixel = 8 bytes. Row padding = 4 bytes.
+
+  GLuint color_space_offset = base::bits::AlignUp(
+      sizeof(cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result),
+      sizeof(uint64_t));
+  GLuint pixels_offset = color_space_offset;
+  GLuint dst_size = dst_info.computeByteSize(dst_row_bytes);
+  GLuint total_size =
+      pixels_offset +
+      base::bits::AlignUp(dst_size, static_cast<GLuint>(sizeof(uint64_t)));
+
+  ExpectedMemoryInfo mem = GetExpectedMappedMemory(total_size);
+
+  std::vector<uint8_t> dst_pixels(dst_row_bytes * dst_info.height(), 0xAA);
+
+  EXPECT_CALL(*command_buffer(), OnFlush())
+      .WillOnce([mem, pixels_offset, dst_size]() {
+        // Write 1 to readback_result (at the beginning of shm).
+        auto* result = reinterpret_cast<
+            cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result*>(mem.ptr);
+        *result = 1;
+
+        // Write test data to the pixel portion of the shared memory.
+        auto src_pixels = mem.span.subspan(pixels_offset, dst_size);
+        // Fill src_pixels with distinct values, e.g. 1 to dst_size
+        for (size_t i = 0; i < dst_size; ++i) {
+          src_pixels[i] = static_cast<uint8_t>(i + 1);
+        }
+      })
+      .RetiresOnSaturation();
+
+  bool success = gl_->ReadbackImagePixels(mailbox, dst_info, dst_row_bytes,
+                                          /*src_x=*/0, /*src_y=*/0,
+                                          /*plane_index=*/0, dst_pixels.data());
+
+  EXPECT_TRUE(success);
+
+  // Expected output:
+  // Row 1 (pixels: 0 to 7) copied from src_pixels (0 to 7): 1, 2, 3, 4, 5, 6,
+  // 7, 8. Row 1 (padding: 8 to 11) untouched: 0xAA, 0xAA, 0xAA, 0xAA. Row 2
+  // (pixels: 12 to 19) copied from src_pixels (12 to 19): 13, 14, 15, 16, 17,
+  // 18, 19, 20. Row 2 (padding: 20 to 23) untouched: 0xAA, 0xAA, 0xAA, 0xAA.
+
+  std::vector<uint8_t> expected_pixels(dst_row_bytes * dst_info.height(), 0xAA);
+  for (int y = 0; y < dst_info.height(); ++y) {
+    for (size_t x = 0; x < dst_info.minRowBytes(); ++x) {
+      size_t dst_idx = y * dst_row_bytes + x;
+      size_t src_idx = y * dst_row_bytes + x;
+      expected_pixels[dst_idx] = static_cast<uint8_t>(src_idx + 1);
+    }
+  }
+
+  EXPECT_EQ(dst_pixels, expected_pixels);
+}
+
+// https://crbug.com/543707066
+TEST_F(RasterImplementationTest, ReadbackImagePixelsAsyncPadding) {
+  gpu::Mailbox mailbox = gpu::Mailbox::Generate();
+  SkImageInfo dst_info = SkImageInfo::MakeN32Premul(2, 2);
+  GLuint dst_row_bytes =
+      12;  // 2 pixels * 4 bytes/pixel = 8 bytes. Row padding = 4 bytes.
+
+  GLuint color_space_offset = base::bits::AlignUp(
+      sizeof(cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result),
+      sizeof(uint64_t));
+  GLuint pixels_offset = color_space_offset;
+  GLuint dst_size = dst_info.computeByteSize(dst_row_bytes);
+  GLuint total_size =
... (truncated)