Force-update unsafe buffers in third_party/blink [chromium/src : main]

0 views
Skip to first unread message

Tom Sepez (Gerrit)

unread,
Aug 5, 2025, 8:04:45 PMAug 5
to Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org

Tom Sepez removed a vote from this change

Removed Commit-Queue+1 by Tom Sepez <tse...@chromium.org>
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: deleteVote
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id154936d2ffa962bd397281b51b72bfc0682286c
Gerrit-Change-Number: 6822198
Gerrit-PatchSet: 5
Gerrit-Owner: Tom Sepez <tse...@chromium.org>
Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>
Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hans Wennborg <ha...@chromium.org>
Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
Gerrit-CC: Hongchan Choi <hong...@chromium.org>
Gerrit-CC: Kaan Icer <ic...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-CC: Urvang Joshi <urv...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Tom Sepez (Gerrit)

unread,
Aug 6, 2025, 3:43:14 PMAug 6
to Nico Weber, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
Attention needed from Nico Weber

Tom Sepez added 1 comment

Patchset-level comments
File-level comment, Patchset 6 (Latest):
Tom Sepez . resolved

Previous mega-cq run hit only patch failed issues, CL now rebased.

Open in Gerrit

Related details

Attention is currently required from:
  • Nico Weber
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
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: Id154936d2ffa962bd397281b51b72bfc0682286c
Gerrit-Change-Number: 6822198
Gerrit-PatchSet: 6
Gerrit-Owner: Tom Sepez <tse...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>
Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hans Wennborg <ha...@chromium.org>
Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
Gerrit-CC: Hongchan Choi <hong...@chromium.org>
Gerrit-CC: Kaan Icer <ic...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-CC: Urvang Joshi <urv...@chromium.org>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Comment-Date: Wed, 06 Aug 2025 19:43:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

James Zern (Gerrit)

unread,
Aug 6, 2025, 7:32:53 PMAug 6
to Tom Sepez, Nico Weber, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
Attention needed from Nico Weber and Tom Sepez

James Zern added 1 comment

File third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
Line 119, Patchset 6 (Latest): return !UNSAFE_TODO(memcmp(blob->bytes(), "RIFF", 4)) &&
James Zern . resolved

Just a general question. In cases like this where we've checked `blob->size()` (line 115) to ensure it's large enough, is there a way to remove the `UNSAFE_TODO`?

Open in Gerrit

Related details

Attention is currently required from:
  • Nico Weber
  • Tom Sepez
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
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: Id154936d2ffa962bd397281b51b72bfc0682286c
Gerrit-Change-Number: 6822198
Gerrit-PatchSet: 6
Gerrit-Owner: Tom Sepez <tse...@chromium.org>
Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>
Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
Gerrit-CC: Daniel Cheng <dch...@chromium.org>
Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
Gerrit-CC: Hans Wennborg <ha...@chromium.org>
Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
Gerrit-CC: Hongchan Choi <hong...@chromium.org>
Gerrit-CC: James Zern <jz...@google.com>
Gerrit-CC: Kaan Icer <ic...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
Gerrit-CC: Nate Chapin <jap...@chromium.org>
Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
Gerrit-CC: Stephen Chenney <sche...@chromium.org>
Gerrit-CC: Urvang Joshi <urv...@chromium.org>
Gerrit-Attention: Tom Sepez <tse...@chromium.org>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Comment-Date: Wed, 06 Aug 2025 23:32:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Tom Sepez (Gerrit)

unread,
Aug 6, 2025, 7:50:26 PMAug 6
to James Zern, Nico Weber, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
Attention needed from James Zern and Nico Weber

Tom Sepez added 1 comment

File third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
Line 119, Patchset 6 (Latest): return !UNSAFE_TODO(memcmp(blob->bytes(), "RIFF", 4)) &&
James Zern . resolved

Just a general question. In cases like this where we've checked `blob->size()` (line 115) to ensure it's large enough, is there a way to remove the `UNSAFE_TODO`?

Tom Sepez
If you wanted to keep the memcmp, you could write
```
  // SAFETY: blob is at least 20 bytes per check above.
  UNSAFE_BUFFERS(memcmp(blob->bytes(), "RIFF", 4))
```
But that's really no better, since if someone removes the check above in a future CL, you're no longer safe. We're at a little bit of a disadvantage that although blob->byteSpan() returns an SkSpan, its not quite as sophisticated as other span implementations. But it should be implicitly convertible to a base span 
```
base::span<const uint8_t> blob_bytes(blob->byteSpan());
if (blob_bytes.size() < 20UL) {
  return false;
}
return blob_bytes.first<4u>() == base::byte_span_from_cstring("RIFF");
```

With the this code, the compiler should be able to optimize out the bounds
check for 4u at compile time, but if someone were to remove the < 20 test, the bounds check would remain and CHECK() if span was too small.

Now to see if any of this actually compiles.

Open in Gerrit

Related details

Attention is currently required from:
  • James Zern
  • Nico Weber
Gerrit-Attention: James Zern <jz...@google.com>
Gerrit-Attention: Nico Weber <tha...@chromium.org>
Gerrit-Comment-Date: Wed, 06 Aug 2025 23:50:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: James Zern <jz...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Tom Sepez (Gerrit)

unread,
Aug 6, 2025, 8:18:48 PMAug 6
to James Zern, Nico Weber, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
Attention needed from James Zern and Nico Weber

Tom Sepez added 1 comment

File third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
Line 119, Patchset 6 (Latest): return !UNSAFE_TODO(memcmp(blob->bytes(), "RIFF", 4)) &&
James Zern . resolved

Just a general question. In cases like this where we've checked `blob->size()` (line 115) to ensure it's large enough, is there a way to remove the `UNSAFE_TODO`?

Tom Sepez
If you wanted to keep the memcmp, you could write
```
  // SAFETY: blob is at least 20 bytes per check above.
  UNSAFE_BUFFERS(memcmp(blob->bytes(), "RIFF", 4))
```
But that's really no better, since if someone removes the check above in a future CL, you're no longer safe. We're at a little bit of a disadvantage that although blob->byteSpan() returns an SkSpan, its not quite as sophisticated as other span implementations. But it should be implicitly convertible to a base span 
```
base::span<const uint8_t> blob_bytes(blob->byteSpan());
if (blob_bytes.size() < 20UL) {
  return false;
}
return blob_bytes.first<4u>() == base::byte_span_from_cstring("RIFF");
```

With the this code, the compiler should be able to optimize out the bounds
check for 4u at compile time, but if someone were to remove the < 20 test, the bounds check would remain and CHECK() if span was too small.

Now to see if any of this actually compiles.

Tom Sepez
And just for the record, it takes a tiny bit more wrangling to satisfy the compiler and its temporary lifetime tracking (doesn't seem to understand that skspan is a borrowed range who's lifetime doesn't matter).
```
  auto sk_blob_bytes = blob->byteSpan();
  base::span<const uint8_t> blob_bytes(sk_blob_bytes);

  if (blob_bytes.size() < 20UL) {
    return false;
  }
  return blob_bytes.first<4u>() == base::byte_span_from_cstring("RIFF") &&
        `
             base::byte_span_with_nul_from_cstring("WEBPVP8");
```
What's interesting is that if I write
```
    blob_bytes.subspan<8u, 8u>() ==
        base::byte_span_with_nul_from_cstring("WEBPVP8");
```
it fails to compile complaining about an 8 vs 7 size mismatch.  Hopefully the spec says that there needs to be a \0 in the 16th byte of the input, or you'd really want to compare a subspan<8u, 7u>.  Note that the new code makes it obvious that you are including the nul in the comparison (just looking at the memcmp I didn't realize that since I didn't realise "wevpvp8" is only 8 chars). So we moved a potential size mismatch to compile time ...
Gerrit-Comment-Date: Thu, 07 Aug 2025 00:18:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Sepez <tse...@chromium.org>
Comment-In-Reply-To: James Zern <jz...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Nico Weber (Gerrit)

unread,
Aug 7, 2025, 9:30:25 AMAug 7
to Tom Sepez, Nico Weber, James Zern, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
Attention needed from James Zern and Tom Sepez

Nico Weber voted and added 2 comments

Votes added by Nico Weber

Code-Review+1

2 comments

Patchset-level comments
Nico Weber . resolved

lg

sorry for the slow review

one suggestion for your consideration, but fine as-is too

File third_party/blink/renderer/platform/graphics/gpu/webgl_image_conversion.cc
Line 8, Patchset 6 (Parent):#endif
Nico Weber . unresolved

there's _so much_ in this file that it might make sense to keep a file-based pragma here

Open in Gerrit

Related details

Attention is currently required from:
  • James Zern
  • Tom Sepez
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Gerrit-Attention: Tom Sepez <tse...@chromium.org>
Gerrit-Attention: James Zern <jz...@google.com>
Gerrit-Comment-Date: Thu, 07 Aug 2025 13:30:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Tom Sepez (Gerrit)

unread,
Aug 7, 2025, 12:29:17 PMAug 7
to Nico Weber, James Zern, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
Attention needed from James Zern and Nico Weber

Tom Sepez added 2 comments

Patchset-level comments
File-level comment, Patchset 7 (Latest):
Tom Sepez . resolved

Needs re-stamp since I reverted the one file as suggested. Thanks!

File third_party/blink/renderer/platform/graphics/gpu/webgl_image_conversion.cc
Nico Weber . resolved

there's _so much_ in this file that it might make sense to keep a file-based pragma here

Tom Sepez

Agreed. Reverting.

Open in Gerrit

Related details

Attention is currently required from:
  • James Zern
  • Nico Weber
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    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: Id154936d2ffa962bd397281b51b72bfc0682286c
    Gerrit-Change-Number: 6822198
    Gerrit-PatchSet: 7
    Gerrit-Owner: Tom Sepez <tse...@chromium.org>
    Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
    Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>
    Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
    Gerrit-CC: Daniel Cheng <dch...@chromium.org>
    Gerrit-CC: Dirk Schulze <dsch...@chromium.org>
    Gerrit-CC: Hans Wennborg <ha...@chromium.org>
    Gerrit-CC: Hirokazu Honda <hi...@chromium.org>
    Gerrit-CC: Hongchan Choi <hong...@chromium.org>
    Gerrit-CC: James Zern <jz...@google.com>
    Gerrit-CC: Kaan Icer <ic...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-CC: Michael Wilson <mjwi...@chromium.org>
    Gerrit-CC: Nate Chapin <jap...@chromium.org>
    Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>
    Gerrit-CC: Stephen Chenney <sche...@chromium.org>
    Gerrit-CC: Urvang Joshi <urv...@chromium.org>
    Gerrit-Attention: James Zern <jz...@google.com>
    Gerrit-Attention: Nico Weber <tha...@chromium.org>
    Gerrit-Comment-Date: Thu, 07 Aug 2025 16:29:03 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Nico Weber <tha...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Nico Weber (Gerrit)

    unread,
    Aug 7, 2025, 12:30:25 PMAug 7
    to Tom Sepez, Nico Weber, James Zern, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
    Attention needed from James Zern and Tom Sepez

    Nico Weber voted Code-Review+1

    Code-Review+1
    Open in Gerrit

    Related details

    Attention is currently required from:
    • James Zern
    • Tom Sepez
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      Gerrit-Attention: Tom Sepez <tse...@chromium.org>
      Gerrit-Attention: James Zern <jz...@google.com>
      Gerrit-Comment-Date: Thu, 07 Aug 2025 16:30:19 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      Tom Sepez (Gerrit)

      unread,
      Aug 7, 2025, 12:38:32 PMAug 7
      to Nico Weber, James Zern, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
      Attention needed from James Zern

      Tom Sepez voted Commit-Queue+2

      Commit-Queue+2
      Open in Gerrit

      Related details

      Attention is currently required from:
      • James Zern
      Gerrit-Attention: James Zern <jz...@google.com>
      Gerrit-Comment-Date: Thu, 07 Aug 2025 16:38:14 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      open
      diffy

      James Zern (Gerrit)

      unread,
      Aug 7, 2025, 12:54:51 PMAug 7
      to Tom Sepez, Nico Weber, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
      Attention needed from Tom Sepez

      James Zern added 1 comment

      File third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
      Line 119, Patchset 6: return !UNSAFE_TODO(memcmp(blob->bytes(), "RIFF", 4)) &&
      James Zern
      And just for the record, it takes a tiny bit more wrangling to satisfy the compiler and its temporary lifetime tracking (doesn't seem to understand that skspan is a borrowed range who's lifetime doesn't matter).
      ```
        auto sk_blob_bytes = blob->byteSpan();
        base::span<const uint8_t> blob_bytes(sk_blob_bytes);
        if (blob_bytes.size() < 20UL) {
          return false;
        }
        return blob_bytes.first<4u>() == base::byte_span_from_cstring("RIFF") &&
              `
                   base::byte_span_with_nul_from_cstring("WEBPVP8");
      ```
      What's interesting is that if I write
      ```
          blob_bytes.subspan<8u, 8u>() ==
              base::byte_span_with_nul_from_cstring("WEBPVP8");
      ```
      it fails to compile complaining about an 8 vs 7 size mismatch.  Hopefully the spec says that there needs to be a \0 in the 16th byte of the input, or you'd really want to compare a subspan<8u, 7u>.  Note that the new code makes it obvious that you are including the nul in the comparison (just looking at the memcmp I didn't realize that since I didn't realise "wevpvp8" is only 8 chars). So we moved a potential size mismatch to compile time ...

      It's an ascii space (`0x20`), not a `'\0'`. So you want `"WEBPVP8 "`.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Tom Sepez
      Gerrit-Attention: Tom Sepez <tse...@chromium.org>
      Gerrit-Comment-Date: Thu, 07 Aug 2025 16:54:39 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      open
      diffy

      Tom Sepez (Gerrit)

      unread,
      Aug 7, 2025, 1:09:14 PMAug 7
      to Nico Weber, James Zern, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
      Attention needed from James Zern

      Tom Sepez added 1 comment

      File third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
      Tom Sepez

      Ah, so there ya go. The version landed had the unsafe_todo(), btw.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • James Zern
      Gerrit-Attention: James Zern <jz...@google.com>
      Gerrit-Comment-Date: Thu, 07 Aug 2025 17:09:02 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: James Zern <jz...@google.com>
      Comment-In-Reply-To: Tom Sepez <tse...@chromium.org>
      satisfied_requirement
      open
      diffy

      Tom Sepez (Gerrit)

      unread,
      Aug 7, 2025, 1:11:18 PMAug 7
      to Nico Weber, James Zern, Chromium LUCI CQ, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org
      File third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
      Tom Sepez

      In other words, if someone were to clumsily loose the space at the end (ha), it would fail to compile in the spanified case.

      Gerrit-Comment-Date: Thu, 07 Aug 2025 17:11:05 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      open
      diffy

      Chromium LUCI CQ (Gerrit)

      unread,
      Aug 7, 2025, 2:23:53 PMAug 7
      to Tom Sepez, Nico Weber, James Zern, Christian Biesinger, chromium...@chromium.org, Daniel Cheng, Dirk Schulze, Hans Wennborg, Kentaro Hara, Hirokazu Honda, Hongchan Choi, Kaan Icer, Nate Chapin, Raphael Kubo da Costa, Stephen Chenney, Urvang Joshi, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, cblume+im...@chromium.org, dmurph+wa...@chromium.org, drott+bl...@chromium.org, edgesto...@microsoft.com, emircan+watch...@chromium.org, enne...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, gavinp...@chromium.org, ipc-securi...@chromium.org, jz...@chromium.org, kinuko+...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, loading...@chromium.org, mbarowsky+wat...@chromium.org, mbarowsky+watc...@chromium.org, mcasas+med...@chromium.org, npm+...@chromium.org, oilpan-rev...@chromium.org, video-networking...@google.com, yigu+...@chromium.org

      Chromium LUCI CQ submitted the change

      Change information

      Commit message:
      Force-update unsafe buffers in third_party/blink

      Update files regardless of presence of #ifdef sections which the
      compiler might not have flagged. Then revert files that fail to
      keep the diffs automated.
      Change-Id: Id154936d2ffa962bd397281b51b72bfc0682286c
      Reviewed-by: Nico Weber <tha...@chromium.org>
      Commit-Queue: Tom Sepez <tse...@chromium.org>
      Cr-Commit-Position: refs/heads/main@{#1498374}
      Files:
      • M third_party/blink/renderer/controller/highest_pmf_reporter_test.cc
      • M third_party/blink/renderer/core/frame/web_frame_test.cc
      • M third_party/blink/renderer/core/loader/resource/resource_loader_code_cache_test.cc
      • M third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
      • M third_party/blink/renderer/modules/credentialmanagement/credential_manager_type_converters_unittest.cc
      • M third_party/blink/renderer/modules/indexeddb/idb_request_test.cc
      • M third_party/blink/renderer/modules/mediarecorder/audio_track_recorder_unittest.cc
      • M third_party/blink/renderer/modules/mediarecorder/media_recorder_handler_unittest.cc
      • M third_party/blink/renderer/modules/mediarecorder/video_track_recorder_unittest.cc
      • M third_party/blink/renderer/platform/audio/delay.cc
      • M third_party/blink/renderer/platform/audio/direct_convolver.cc
      • M third_party/blink/renderer/platform/audio/fft_frame.cc
      • M third_party/blink/renderer/platform/audio/pffft/fft_frame_pffft.cc
      • M third_party/blink/renderer/platform/audio/push_pull_fifo.cc
      • M third_party/blink/renderer/platform/audio/vector_math.cc
      • M third_party/blink/renderer/platform/audio/vector_math_test.cc
      • M third_party/blink/renderer/platform/bindings/parkable_string_test.cc
      • M third_party/blink/renderer/platform/graphics/deferred_image_decoder_test.cc
      • M third_party/blink/renderer/platform/graphics/gpu/drawing_buffer.cc
      • M third_party/blink/renderer/platform/graphics/gpu/webgl_image_conversion_test.cc
      • M third_party/blink/renderer/platform/graphics/gpu/webgpu_swap_buffer_provider_test.cc
      • M third_party/blink/renderer/platform/graphics/paint/paint_controller_debug_data.cc
      • M third_party/blink/renderer/platform/heap/test/heap_test.cc
      • M third_party/blink/renderer/platform/image-decoders/avif/crabbyavif_image_decoder.cc
      • M third_party/blink/renderer/platform/image-decoders/avif/crabbyavif_image_decoder_test.cc
      • M third_party/blink/renderer/platform/image-decoders/image_decoder_base_test.cc
      • M third_party/blink/renderer/platform/image-decoders/image_frame_test.cc
      • M third_party/blink/renderer/platform/image-decoders/jpeg/jpeg_image_decoder.cc
      • M third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc
      • M third_party/blink/renderer/platform/image-decoders/webp/webp_image_decoder.cc
      • M third_party/blink/renderer/platform/p2p/filtering_network_manager_test.cc
      • M third_party/blink/renderer/platform/peerconnection/rtc_video_encoder.cc
      • M third_party/blink/renderer/platform/peerconnection/video_encoder_state_observer_impl.cc
      • M third_party/blink/renderer/platform/wtf/atomic_operations.cc
      • M third_party/blink/renderer/platform/wtf/date_math.cc
      • M third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc
      • M third_party/blink/renderer/platform/wtf/text/string_view_test.cc
      Change size: XL
      Delta: 37 files changed, 515 insertions(+), 571 deletions(-)
      Branch: refs/heads/main
      Submit Requirements:
      • requirement satisfiedCode-Review: +1 by Nico Weber
      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: Id154936d2ffa962bd397281b51b72bfc0682286c
      Gerrit-Change-Number: 6822198
      Gerrit-PatchSet: 8
      Gerrit-Owner: Tom Sepez <tse...@chromium.org>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Nico Weber <tha...@chromium.org>
      Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>
      Gerrit-CC: Christian Biesinger <cbies...@chromium.org>
      open
      diffy
      satisfied_requirement
      Reply all
      Reply to author
      Forward
      0 new messages