[webkit-reviews] review granted: [Bug 239058] [WebGPU] Use checked arithmetic : [Attachment 457228] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 239058: [WebGPU] Use checked arithmetic
https://bugs.webkit.org/show_bug.cgi?id=239058

Attachment 457228: Patch

https://bugs.webkit.org/attachment.cgi?id=457228&action=review




--- Comment #4 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 457228
  --> https://bugs.webkit.org/attachment.cgi?id=457228
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457228&action=review

> Source/WebGPU/WebGPU/Buffer.mm:180
> +    auto endOffset = CheckedSize(offset) + CheckedSize(rangeSize);

auto endOffset = checkedSum<size_t>(offset, rangeSize);

> Source/WebGPU/WebGPU/Buffer.mm:194
>      auto rangeSize = size;

here it would be more more appropriate to have the rangeSize local as a
checked.

so you'd do:
auto rangeSize = checkedRangeSize(m_size, size, offset)

and implementation would use Checked instead of static asserts. The static
asserts are a bit confusing, as the first one asserts a relatively well-known
language-level fact (unsigned is unsigned) and the second one asserts a
language-level fact that "technically may not always hold". 
E.g. static asserting a user-level construct is "assert that user is holding
this correctly", which is ok.
 When one starts to assert language level constructs, it is like "assert that
we live in the universe we think we live in", which starts to get strange.

> Source/WebGPU/WebGPU/Buffer.mm:228
> +    auto end = CheckedUint64(CheckedSize(offset) + CheckedSize(rangeSize));

I think if your m_size is uint64, you are interested in the calculation in that
domain.
so you could just do
auto end = checkedSum<uint64_t>(offset, rangeSize)

> Source/WebGPU/WebGPU/Texture.mm:2783
> +	   auto bytesBeforeLastImage = bytesPerImage *
(CheckedUint32(copyExtent.depthOrArrayLayers) - 1);

Your patch is mostly written in style of
 "cast all the elements of an equation into checked"
However, there as still some that are written in style of 
 "cast only the innermost elements into checked and let the precedence rules
and implicit type conversion cast every other element correctly"

You do the latter since you don't want to explicitly cast - 1 to -
CheckedUint32(1).
I'd think it's a bit more eloquent to use the latter rule and trust that when
verifying and writing this, the developers know the precedence rules

> Source/WebGPU/WebGPU/Texture.mm:2793
> +	       requiredBytesInCopy += bytesInLastRow.value();

maybe doesn't need .value()