Bug 239058 – [WebGPU] Use checked arithmetic

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Myles C. Maxfield]

.

[Myles C. Maxfield]

Created attachment 457226 [details]
Patch

[Myles C. Maxfield]

Created attachment 457227 [details]
Patch

[Myles C. Maxfield]

Created attachment 457228 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 457228 [details]
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()

[Myles C. Maxfield]

Committed r292757 (249541@trunk): <https://commits.webkit.org/249541@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/91603903>