[fuchsia][flatland] Validate image size and layout [chromium/src : main]

0 views
Skip to first unread message

David Song (Gerrit)

unread,
Jul 15, 2026, 1:56:32 PM (8 days ago) Jul 15
to Zijie He, Sergey Ulanov, Chromium LUCI CQ, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org
Attention needed from Sergey Ulanov and Zijie He

David Song voted

Auto-Submit+1
Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Sergey Ulanov
  • Zijie He
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
Gerrit-Change-Number: 8007838
Gerrit-PatchSet: 10
Gerrit-Owner: David Song <winter...@google.com>
Gerrit-Reviewer: David Song <winter...@google.com>
Gerrit-Reviewer: Sergey Ulanov <ser...@chromium.org>
Gerrit-Reviewer: Zijie He <zij...@google.com>
Gerrit-CC: David Worsham <dwor...@google.com>
Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>
Gerrit-Attention: Sergey Ulanov <ser...@chromium.org>
Gerrit-Attention: Zijie He <zij...@google.com>
Gerrit-Comment-Date: Wed, 15 Jul 2026 17:56:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

David Song (Gerrit)

unread,
Jul 15, 2026, 5:21:34 PM (8 days ago) Jul 15
to Sergey Ulanov, Zijie He, Chromium LUCI CQ, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org
Attention needed from Zijie He

David Song removed Sergey Ulanov from this change

Deleted Reviewers:
  • Sergey Ulanov
Open in Gerrit

Related details

Attention is currently required from:
  • Zijie He
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: deleteReviewer
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
Gerrit-Change-Number: 8007838
Gerrit-PatchSet: 11
Gerrit-Owner: David Song <winter...@google.com>
Gerrit-Reviewer: David Song <winter...@google.com>
Gerrit-Reviewer: Zijie He <zij...@google.com>
Gerrit-CC: David Worsham <dwor...@google.com>
Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>
Gerrit-Attention: Zijie He <zij...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Zijie He (Gerrit)

unread,
Jul 15, 2026, 7:54:28 PM (8 days ago) Jul 15
to David Song, Chromium LUCI CQ, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org
Attention needed from David Song

Zijie He voted and added 9 comments

Votes added by Zijie He

Code-Review+1

9 comments

Patchset-level comments
File-level comment, Patchset 11 (Latest):
Zijie He . resolved

Mainly some nits, overall LGTM. Thank you.

File ui/ozone/platform/flatland/flatland_sysmem_buffer_collection.cc
Line 88, Patchset 11 (Latest): static_cast<size_t>(constraints.min_bytes_per_row()), stride_from_size);
Zijie He . unresolved

Nit, just call checked_stride.ValueOrDie() instead of stride_from_size. But see my another suggestion below.

Line 90, Patchset 11 (Latest): base::CheckedNumeric<size_t> checked_rounded_stride = stride_value;
Zijie He . unresolved

I think you can just say checked_stride.Max(constraints.min_bytes_per_row()).

Line 93, Patchset 11 (Latest): if (!checked_rounded_stride.IsValid()) {
return std::nullopt;
}
Zijie He . unresolved

Nit, I think the beautiful thing of CheckedNumeric is that you can continue the calculation but check IsValid() at the very end.

Line 104, Patchset 11 (Latest): base::CheckedNumeric<size_t> checked_plane_size = stride;
Zijie He . unresolved

Nit, you can copy a CheckedNumeric from another CheckedNumeric without needing to do ValueOrDie() here.

Line 119, Patchset 11 (Latest): layout.stride = stride;
Zijie He . unresolved

Ditto, you may just use ValueOrDie() here to make the assumption more obvious.

Line 123, Patchset 11 (Latest): if (format == viz::MultiPlaneFormat::kNV12) {
Zijie He . unresolved

It looks like the uv_plane_offset and uv_plane_size are only calculated and used for NV12, I may suggest adding a comment here to explain it.

Line 361, Patchset 11 (Latest): DCHECK(sysmem_token);
Zijie He . unresolved

I think you may also want to log and return false here.

Line 500, Patchset 11 (Latest): .has_value()) {
Zijie He . unresolved

Nit, this .has_value() call seems not necessary.

Open in Gerrit

Related details

Attention is currently required from:
  • David Song
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
    Gerrit-Change-Number: 8007838
    Gerrit-PatchSet: 11
    Gerrit-Owner: David Song <winter...@google.com>
    Gerrit-Reviewer: David Song <winter...@google.com>
    Gerrit-Reviewer: Zijie He <zij...@google.com>
    Gerrit-CC: David Worsham <dwor...@google.com>
    Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>
    Gerrit-Attention: David Song <winter...@google.com>
    Gerrit-Comment-Date: Wed, 15 Jul 2026 23:54:14 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    David Song (Gerrit)

    unread,
    Jul 16, 2026, 4:30:21 PM (7 days ago) Jul 16
    to Zijie He, Chromium LUCI CQ, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org

    David Song voted and added 8 comments

    Votes added by David Song

    Auto-Submit+1
    Commit-Queue+1

    8 comments

    File ui/ozone/platform/flatland/flatland_sysmem_buffer_collection.cc
    Line 88, Patchset 11: static_cast<size_t>(constraints.min_bytes_per_row()), stride_from_size);
    Zijie He . resolved

    Nit, just call checked_stride.ValueOrDie() instead of stride_from_size. But see my another suggestion below.

    David Song

    Changed to use `CheckedNumeric` throughout, and checked at the end.

    Line 90, Patchset 11: base::CheckedNumeric<size_t> checked_rounded_stride = stride_value;
    Zijie He . resolved

    I think you can just say checked_stride.Max(constraints.min_bytes_per_row()).

    David Song

    Done

    Line 93, Patchset 11: if (!checked_rounded_stride.IsValid()) {
    return std::nullopt;
    }
    Zijie He . resolved

    Nit, I think the beautiful thing of CheckedNumeric is that you can continue the calculation but check IsValid() at the very end.

    David Song

    I didn't look at it closely before, this is quite cool!

    Line 104, Patchset 11: base::CheckedNumeric<size_t> checked_plane_size = stride;
    Zijie He . resolved

    Nit, you can copy a CheckedNumeric from another CheckedNumeric without needing to do ValueOrDie() here.

    David Song

    Done

    Line 119, Patchset 11: layout.stride = stride;
    Zijie He . resolved

    Ditto, you may just use ValueOrDie() here to make the assumption more obvious.

    David Song

    Done

    Line 123, Patchset 11: if (format == viz::MultiPlaneFormat::kNV12) {
    Zijie He . resolved

    It looks like the uv_plane_offset and uv_plane_size are only calculated and used for NV12, I may suggest adding a comment here to explain it.

    David Song

    Added in the struct definition.

    Line 361, Patchset 11: DCHECK(sysmem_token);
    Zijie He . resolved

    I think you may also want to log and return false here.

    David Song

    Yup, missed that. Done.

    Line 500, Patchset 11: .has_value()) {
    Zijie He . resolved

    Nit, this .has_value() call seems not necessary.

    David Song

    Done

    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
      Gerrit-Change-Number: 8007838
      Gerrit-PatchSet: 13
      Gerrit-Owner: David Song <winter...@google.com>
      Gerrit-Reviewer: David Song <winter...@google.com>
      Gerrit-Reviewer: Zijie He <zij...@google.com>
      Gerrit-CC: David Worsham <dwor...@google.com>
      Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>
      Gerrit-Comment-Date: Thu, 16 Jul 2026 20:30:08 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Zijie He <zij...@google.com>
      satisfied_requirement
      open
      diffy

      David Song (Gerrit)

      unread,
      Jul 20, 2026, 2:18:06 PM (3 days ago) Jul 20
      to Zijie He, Chromium LUCI CQ, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org

      David Song voted Commit-Queue+2

      Commit-Queue+2
      Open in Gerrit

      Related details

      Attention set is empty
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
      Gerrit-Change-Number: 8007838
      Gerrit-PatchSet: 13
      Gerrit-Owner: David Song <winter...@google.com>
      Gerrit-Reviewer: David Song <winter...@google.com>
      Gerrit-Reviewer: Zijie He <zij...@google.com>
      Gerrit-CC: David Worsham <dwor...@google.com>
      Gerrit-CC: Robert Kroeger <rjkr...@chromium.org>
      Gerrit-Comment-Date: Mon, 20 Jul 2026 18:17:51 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Chromium LUCI CQ (Gerrit)

      unread,
      Jul 20, 2026, 3:15:45 PM (3 days ago) Jul 20
      to David Song, Zijie He, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org

      Chromium LUCI CQ submitted the change with unreviewed changes

      Unreviewed changes

      11 is the latest approved patch-set.
      The change was submitted with unreviewed changes in the following files:

      ```
      The name of the file: ui/ozone/platform/flatland/flatland_sysmem_buffer_collection.cc
      Insertions: 30, Deletions: 42.

      @@ -59,6 +59,9 @@
         size_t stride = 0;
         size_t plane_offset = 0;
         size_t plane_size = 0;
      +
      +  // uv_plane_offset and uv_plane_size are only calculated and used for NV12
      +  // (2-plane YUV).
         size_t uv_plane_offset = 0;
         size_t uv_plane_size = 0;
       };
      @@ -79,59 +82,42 @@
       
         base::CheckedNumeric<size_t> checked_stride = size.width();
         checked_stride *= GetBytesPerPixel(format);
      -  if (!checked_stride.IsValid()) {
      -    return std::nullopt;
      -  }
      +  checked_stride =
      +      checked_stride.Max(static_cast<size_t>(constraints.min_bytes_per_row()));
       
      -  size_t stride_from_size = checked_stride.ValueOrDie();
      -  size_t stride_value = std::max(
      -      static_cast<size_t>(constraints.min_bytes_per_row()), stride_from_size);
         size_t divisor = constraints.bytes_per_row_divisor();
      -  base::CheckedNumeric<size_t> checked_rounded_stride = stride_value;
         if (divisor > 0) {
      -    checked_rounded_stride += divisor - 1;
      -    if (!checked_rounded_stride.IsValid()) {
      -      return std::nullopt;
      -    }
      -    size_t rounded_stride_value = checked_rounded_stride.ValueOrDie();
      -    checked_rounded_stride = (rounded_stride_value / divisor) * divisor;
      +    checked_stride = ((checked_stride + divisor - 1) / divisor) * divisor;
         }
      -  if (!checked_rounded_stride.IsValid()) {
      -    return std::nullopt;
      -  }
      -  size_t stride = checked_rounded_stride.ValueOrDie();
       
      -  base::CheckedNumeric<size_t> checked_plane_size = stride;
      -  checked_plane_size *= size.height();
      -  if (!checked_plane_size.IsValid()) {
      -    return std::nullopt;
      -  }
      -  size_t plane_size = checked_plane_size.ValueOrDie();
      +  base::CheckedNumeric<size_t> checked_plane_size =
      +      checked_stride * size.height();
      +  base::CheckedNumeric<size_t> checked_plane_end =
      +      checked_plane_size + vmo_usable_start;
       
      -  base::CheckedNumeric<size_t> checked_plane_end = vmo_usable_start;
      -  checked_plane_end += plane_size;
      +  // Checking validity of `checked_plane_end` also implicitly validates
      +  // `checked_plane_size` and `checked_stride`, because they are chained
      +  // together.
         if (!checked_plane_end.IsValid() ||
             checked_plane_end.ValueOrDie() > buffer_size) {
           return std::nullopt;
         }
       
         BufferLayout layout;
      -  layout.stride = stride;
      +  layout.stride = checked_stride.ValueOrDie();
         layout.plane_offset = vmo_usable_start;
      -  layout.plane_size = plane_size;
      +  layout.plane_size = checked_plane_size.ValueOrDie();

       
         if (format == viz::MultiPlaneFormat::kNV12) {
      -    base::CheckedNumeric<size_t> checked_uv_plane_offset = vmo_usable_start;
      -    checked_uv_plane_offset += plane_size;
      -    base::CheckedNumeric<size_t> checked_uv_plane_size = stride;
      -    checked_uv_plane_size *= (static_cast<size_t>(size.height()) + 1) / 2;
      -    if (!checked_uv_plane_offset.IsValid() ||
      -        !checked_uv_plane_size.IsValid()) {
      -      return std::nullopt;
      -    }
      +    base::CheckedNumeric<size_t> checked_uv_plane_offset = checked_plane_end;
      +    base::CheckedNumeric<size_t> checked_uv_plane_size =
      +        checked_stride * ((static_cast<size_t>(size.height()) + 1) / 2);
      +    base::CheckedNumeric<size_t> checked_uv_plane_end =
      +        checked_uv_plane_offset + checked_uv_plane_size;
       
      -    base::CheckedNumeric<size_t> checked_uv_plane_end = checked_uv_plane_offset;
      -    checked_uv_plane_end += checked_uv_plane_size;
      +    // Checking validity of `checked_uv_plane_end` also implicitly validates
      +    // `checked_uv_plane_size` and `checked_uv_plane_offset` because they are
      +    // chained together.
           if (!checked_uv_plane_end.IsValid() ||
               checked_uv_plane_end.ValueOrDie() > buffer_size) {
             return std::nullopt;
      @@ -358,7 +344,10 @@
         if (size.IsEmpty()) {
           // Buffer collection that doesn't have explicit size is expected to be
           // shared with other participants, who will determine the actual image size.
      -    DCHECK(sysmem_token);
      +    if (!sysmem_token) {
      +      LOG(ERROR) << "Sysmem token is required for empty size.";
      +      return false;
      +    }
       
           // Set nominal size of 1x1, which will be used only for
           // vkSetBufferCollectionConstraintsFUCHSIA(). The actual size of the
      @@ -494,10 +483,9 @@
       
         // Verify that the image size and layout fit within the allocated buffer.
         if (!CalculateBufferLayout(
      -           buffers_info_.settings().image_format_constraints(), size, format_,
      -           buffers_info_.buffers()[buffer_index].vmo_usable_start(),
      -           buffer_size_)
      -           .has_value()) {
      +          buffers_info_.settings().image_format_constraints(), size, format_,
      +          buffers_info_.buffers()[buffer_index].vmo_usable_start(),
      +          buffer_size_)) {
           LOG(ERROR) << "Requested VkImage size " << size.ToString()
                      << " exceeds the allocated sysmem buffer size.";
           return false;
      ```

      Change information

      Commit message:
      [fuchsia][flatland] Validate image size and layout

      Validate gfx::Size and format in Initialize, CreateNativePixmap, and
      CreateVkImage to prevent buffer overflows.

      - Unify size validation and layout calculation in the
        CalculateBufferLayout helper using checked math.
      - Verify that the computed image layout fits within the allocated buffer
        size.
      - Keep layout boundary checks and early size/format checks as graceful
        failures (returning nullptr or false) to prevent DoS from untrusted
        inputs.
      - Convert Vulkan memory requirements checks to CHECKs (crash on
        unexpected system values).

      TAG=agy
      CONV=c64f46f9-7333-4bcf-8497-788ae27b1ce7
      Bug: 500576940
      Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
      Auto-Submit: David Song <winter...@google.com>
      Commit-Queue: David Song <winter...@google.com>
      Reviewed-by: Zijie He <zij...@google.com>
      Cr-Commit-Position: refs/heads/main@{#1664843}
      Files:
      • M ui/ozone/platform/flatland/flatland_sysmem_buffer_collection.cc
      Change size: M
      Delta: 1 file changed, 132 insertions(+), 27 deletions(-)
      Branch: refs/heads/main
      Submit Requirements:
      • requirement satisfiedCode-Review: +1 by Zijie He
      Open in Gerrit
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: merged
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I5a89a49b8be9a165d3da9b78c2cf1d56a6b697eb
      Gerrit-Change-Number: 8007838
      Gerrit-PatchSet: 14
      Gerrit-Owner: David Song <winter...@google.com>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: David Song <winter...@google.com>
      Gerrit-Reviewer: Zijie He <zij...@google.com>
      open
      diffy
      satisfied_requirement

      David Song (Gerrit)

      unread,
      Jul 21, 2026, 6:33:43 PM (2 days ago) Jul 21
      to Chromium LUCI CQ, Zijie He, chromium...@chromium.org, David Worsham, Robert Kroeger, emi...@google.com, fuchsia...@chromium.org, ozone-...@chromium.org, spang...@chromium.org

      David Song has created a revert of this change

      Related details

      Attention set is empty
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: revert
      satisfied_requirement
      open
      diffy
      Reply all
      Reply to author
      Forward
      0 new messages