[webkit-reviews] review denied: [Bug 196793] [Web GPU] Prevent lossy 64-bit to 32-bit narrowing conversions during Metal function calls on 32-bit platforms : [Attachment 367181] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 11 09:19:05 PDT 2019


Darin Adler <darin at apple.com> has denied Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 196793: [Web GPU] Prevent lossy 64-bit to 32-bit narrowing conversions
during Metal function calls on 32-bit platforms
https://bugs.webkit.org/show_bug.cgi?id=196793

Attachment 367181: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 367181
  --> https://bugs.webkit.org/attachment.cgi?id=367181
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?


More information about the webkit-reviews mailing list