Bug 239058 - [WebGPU] Use checked arithmetic
Summary: [WebGPU] Use checked arithmetic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-10 19:49 PDT by Myles C. Maxfield
Modified: 2022-04-11 20:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (28.22 KB, patch)
2022-04-10 19:55 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (28.68 KB, patch)
2022-04-10 19:56 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (27.64 KB, patch)
2022-04-10 20:31 PDT, Myles C. Maxfield
kkinnunen: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2022-04-10 19:49:18 PDT
.
Comment 1 Myles C. Maxfield 2022-04-10 19:55:07 PDT
Created attachment 457226 [details]
Patch
Comment 2 Myles C. Maxfield 2022-04-10 19:56:38 PDT
Created attachment 457227 [details]
Patch
Comment 3 Myles C. Maxfield 2022-04-10 20:31:26 PDT
Created attachment 457228 [details]
Patch
Comment 4 Kimmo Kinnunen 2022-04-11 02:13:51 PDT
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()
Comment 5 Myles C. Maxfield 2022-04-11 20:00:29 PDT
Committed r292757 (249541@trunk): <https://commits.webkit.org/249541@trunk>
Comment 6 Radar WebKit Bug Importer 2022-04-11 20:01:16 PDT
<rdar://problem/91603903>