Bug 196793 – [Web GPU] Prevent lossy 64-bit to 32-bit narrowing conversions during Metal function calls on 32-bit platforms

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

[Justin Fan]

[Web GPU] Prevent narrowing conversions during Metal function calls on 32-bit platforms

[Justin Fan]

<rdar://problem/49779960>

[Justin Fan]

Created attachment 367181 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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.

[Justin Fan]

(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.

[Justin Fan]

Created attachment 367353 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 367353 [details]
Patch for landing

Clearing flags on attachment: 367353

Committed r244235: <https://trac.webkit.org/changeset/244235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Justin Fan]

(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.