Bug 196793 - [Web GPU] Prevent lossy 64-bit to 32-bit narrowing conversions during Metal function calls on 32-bit platforms
Summary: [Web GPU] Prevent lossy 64-bit to 32-bit narrowing conversions during Metal f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-10 16:34 PDT by Justin Fan
Modified: 2019-04-15 14:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (17.18 KB, patch)
2019-04-10 17:14 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (17.73 KB, patch)
2019-04-12 16:00 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-04-10 16:34:47 PDT
[Web GPU] Prevent narrowing conversions during Metal function calls on 32-bit platforms
Comment 1 Justin Fan 2019-04-10 16:35:08 PDT
<rdar://problem/49779960>
Comment 2 Justin Fan 2019-04-10 17:14:22 PDT
Created attachment 367181 [details]
Patch
Comment 3 Darin Adler 2019-04-11 09:19:05 PDT
Comment on attachment 367181 [details]
Patch

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

Looks great.

I think the code would be cleaner if we used local variables and whenever possible put the static_cast to NSUInteger right next to the bounds check.

review- because we should not be using BoundsChecker directly (see comments below).

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupMetal.mm:68
> +    if (!WTF::BoundsChecker<NSUInteger, uint64_t>::inBounds(bufferBinding.offset)) {

BoundsChecker is intended for use within the CheckedArithmetic.h header, not use outside of it. It’s also unfortunate that this hard-codes the type uint64_t, when it would be better to safely deduce the type from its argument. The way to write this is:

    !WTF::isInBounds<NSUInteger>(bufferBinding.offset)

Or we could combine the bounds check with the typecast by using WTF::convertSafely.

The comment saying "MTLBuffer size (NSUInteger) is 32 bits on some platforms" seems slightly too oblique. While remaining brief, I think the comment needs to explain why a check here is both necessary and sufficient. I guess I’d want to see a comment making it clear how the typecast needed below is always safe because of this bounds check. Ideally the structure would have an NSUInteger in it, but that may be impractical if it’s a platform-independent structure.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:68
> +    // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
> +    if (!WTF::BoundsChecker<NSUInteger, uint64_t>::inBounds(descriptor.size)) {

Same comments here about the comment and use of BoundsChecker (and in other cases below as well, I won’t comment on each one).

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:97
> +    return adoptRef(*new GPUBuffer(WTFMove(mtlBuffer), static_cast<size_t>(descriptor.size), usage, WTFMove(device)));

Unclear why casting to size_t is necessary, and why it’s safe. Is it safe because size_t is always the same as or bigger than NSUInteger? If so, that’s not obvious.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:111
> +    auto srcLength = checkedSum<NSUInteger>(size, srcOffset);
> +    auto dstLength = checkedSum<NSUInteger>(size, dstOffset);

I think that this is non-obvious and is worth a brief comment. The fact each of these calls takes care of all three checks, checking both the arguments for whether they fit in NSUInteger and the result for overflow of NSUInteger range as well, is non-obvious; it’s easy to think that a function template that takes NSUInteger would require NSUInteger arguments, but, hooray, it’s better designed than that, and it doesn’t. That’s important because below, we do typecasts that would otherwise be unsafe. The code is correct, but we want it to be obvious as well so we don’t get confused and break it later.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:360
> +    auto totalOffset = checkedSum<NSUInteger>(firstIndexOffset, m_indexBufferOffset);

Same comment as above, although here there are no casts below. But it’s the same confusion about whether it’s a problem that the arguments are not NSUInteger.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:344
> +        // FIXME: Ensure offset < buffer's stride + format's data size.

Why does this comment not say "After adding more vertex formats", since the original comment did before? Was that unnecessary or incorrect?
Comment 4 Darin Adler 2019-04-11 09:19:58 PDT
Comment on attachment 367181 [details]
Patch

I guess instead I should say review+, but please change to use isInBounds instead of BoundsChecker. Please consider my other comments as well.
Comment 5 Justin Fan 2019-04-11 11:17:12 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 367181 [details]
> Patch
> 
> I guess instead I should say review+, but please change to use isInBounds
> instead of BoundsChecker. Please consider my other comments as well.

Thanks, Darin! Super helpful. I definitely wasn't sure about BoundsChecker.
Comment 6 Justin Fan 2019-04-12 16:00:43 PDT
Created attachment 367353 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-04-12 16:40:59 PDT
Comment on attachment 367353 [details]
Patch for landing

Clearing flags on attachment: 367353

Committed r244235: <https://trac.webkit.org/changeset/244235>
Comment 8 WebKit Commit Bot 2019-04-12 16:41:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2019-04-13 08:09:42 PDT
Comment on attachment 367353 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:346
> +        NSUInteger attributeOffset = 0;
> +        if (!WTF::convertSafely<NSUInteger, uint64_t>(attributes[i].offset, attributeOffset)) {

One last comment. The great thing about convertSafely is that it can deduce type from its arguments, so this can just be written:

    if (!WTF::convertSafely(attributes[i].offset, attributeOffset)) {

I suggest you take advantage of that by removing the explicit template arguments. Also, no need to initialize to 0, but I suppose that’s a good "initialize everything" habit.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:368
> +        NSUInteger inputStride = 0;
> +        if (!WTF::convertSafely<NSUInteger, uint64_t>(inputs[j].stride, inputStride)) {

Ditto.
Comment 10 Justin Fan 2019-04-15 14:59:25 PDT
(In reply to Darin Adler from comment #9)
> Comment on attachment 367353 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367353&action=review
> 
> > Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:346
> > +        NSUInteger attributeOffset = 0;
> > +        if (!WTF::convertSafely<NSUInteger, uint64_t>(attributes[i].offset, attributeOffset)) {
> 
> One last comment. The great thing about convertSafely is that it can deduce
> type from its arguments, so this can just be written:
> 
>     if (!WTF::convertSafely(attributes[i].offset, attributeOffset)) {
> 
> I suggest you take advantage of that by removing the explicit template
> arguments. Also, no need to initialize to 0, but I suppose that’s a good
> "initialize everything" habit.
> 
> > Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:368
> > +        NSUInteger inputStride = 0;
> > +        if (!WTF::convertSafely<NSUInteger, uint64_t>(inputs[j].stride, inputStride)) {
> 
> Ditto.

Updated in https://trac.webkit.org/changeset/244297/webkit.