[Web GPU] Prevent narrowing conversions during Metal function calls on 32-bit platforms
authorjustin_fan@apple.com <justin_fan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Apr 2019 23:40:53 +0000 (23:40 +0000)
committerjustin_fan@apple.com <justin_fan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Apr 2019 23:40:53 +0000 (23:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196793

Reviewed by Darin Adler.

On 32-bit platforms, NSUInteger is 32-bit, which limits certain Web GPU parameters.
Ensure that valid parameters are properly converted to NSUInteger for Metal calls, regardless of platform.

* platform/graphics/gpu/GPUBuffer.h:
(WebCore::GPUBuffer::byteLength const):
* platform/graphics/gpu/cocoa/GPUBindGroupMetal.mm:
(WebCore::tryGetResourceAsBufferBinding):
(WebCore::setBufferOnEncoder):
* platform/graphics/gpu/cocoa/GPUBufferMetal.mm:
(WebCore::GPUBuffer::validateBufferUsage):
(WebCore::GPUBuffer::tryCreate):
(WebCore::GPUBuffer::GPUBuffer):
(WebCore::GPUBuffer::setSubData):
* platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:
(WebCore::GPUCommandBuffer::copyBufferToBuffer):
(WebCore::GPUCommandBuffer::copyBufferToTexture):
(WebCore::GPUCommandBuffer::copyTextureToBuffer):
* platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:
(WebCore::GPURenderPassEncoder::drawIndexed):
* platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:
(WebCore::trySetInputStateForPipelineDescriptor):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244235 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/gpu/GPUBuffer.h
Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupMetal.mm
Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm
Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm
Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm
Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm

index d85ceba..ad6357d 100644 (file)
@@ -1,3 +1,32 @@
+2019-04-12  Justin Fan  <justin_fan@apple.com>
+
+        [Web GPU] Prevent narrowing conversions during Metal function calls on 32-bit platforms
+        https://bugs.webkit.org/show_bug.cgi?id=196793
+
+        Reviewed by Darin Adler.
+
+        On 32-bit platforms, NSUInteger is 32-bit, which limits certain Web GPU parameters. 
+        Ensure that valid parameters are properly converted to NSUInteger for Metal calls, regardless of platform.
+
+        * platform/graphics/gpu/GPUBuffer.h:
+        (WebCore::GPUBuffer::byteLength const):
+        * platform/graphics/gpu/cocoa/GPUBindGroupMetal.mm:
+        (WebCore::tryGetResourceAsBufferBinding):
+        (WebCore::setBufferOnEncoder):
+        * platform/graphics/gpu/cocoa/GPUBufferMetal.mm:
+        (WebCore::GPUBuffer::validateBufferUsage):
+        (WebCore::GPUBuffer::tryCreate):
+        (WebCore::GPUBuffer::GPUBuffer):
+        (WebCore::GPUBuffer::setSubData):
+        * platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:
+        (WebCore::GPUCommandBuffer::copyBufferToBuffer):
+        (WebCore::GPUCommandBuffer::copyBufferToTexture):
+        (WebCore::GPUCommandBuffer::copyTextureToBuffer):
+        * platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:
+        (WebCore::GPURenderPassEncoder::drawIndexed):
+        * platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:
+        (WebCore::trySetInputStateForPipelineDescriptor):
+
 2019-04-12  Ross Kirsling  <ross.kirsling@sony.com>
 
         Unreviewed fix for non-unified build.
index 55dbea9..e46a9a4 100644 (file)
@@ -71,7 +71,7 @@ public:
     static RefPtr<GPUBuffer> tryCreate(Ref<GPUDevice>&&, const GPUBufferDescriptor&);
 
     PlatformBuffer *platformBuffer() const { return m_platformBuffer.get(); }
-    uint64_t byteLength() const { return m_byteLength; }
+    size_t byteLength() const { return m_byteLength; }
     bool isTransferSource() const { return m_usage.contains(GPUBufferUsage::Flags::TransferSource); }
     bool isTransferDestination() const { return m_usage.contains(GPUBufferUsage::Flags::TransferDestination); }
     bool isIndex() const { return m_usage.contains(GPUBufferUsage::Flags::Index); }
@@ -110,7 +110,7 @@ private:
 
     static bool validateBufferUsage(const GPUDevice&, OptionSet<GPUBufferUsage::Flags>);
 
-    GPUBuffer(PlatformBufferSmartPtr&&, uint64_t, OptionSet<GPUBufferUsage::Flags>, Ref<GPUDevice>&&);
+    GPUBuffer(PlatformBufferSmartPtr&&, size_t, OptionSet<GPUBufferUsage::Flags>, Ref<GPUDevice>&&);
 
     JSC::ArrayBuffer* stagingBufferForRead();
     JSC::ArrayBuffer* stagingBufferForWrite();
@@ -132,7 +132,7 @@ private:
     RefPtr<PendingMappingCallback> m_mappingCallback;
     DeferrableTask<Timer> m_mappingCallbackTask;
 
-    uint64_t m_byteLength;
+    size_t m_byteLength;
     OptionSet<GPUBufferUsage::Flags> m_usage;
     unsigned m_numScheduledCommandBuffers { 0 };
 };
index 7c4111d..da2c20d 100644 (file)
@@ -35,6 +35,7 @@
 #import "Logging.h"
 #import <Metal/Metal.h>
 #import <wtf/BlockObjCExceptions.h>
+#import <wtf/CheckedArithmetic.h>
 #import <wtf/Optional.h>
 
 namespace WebCore {
@@ -63,6 +64,11 @@ static Optional<GPUBufferBinding> tryGetResourceAsBufferBinding(const GPUBinding
         LOG(WebGPU, "%s: Invalid MTLBuffer in GPUBufferBinding!", functionName);
         return WTF::nullopt;
     }
+    // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
+    if (!WTF::isInBounds<NSUInteger>(bufferBinding.offset)) {
+        LOG(WebGPU, "%s: Buffer offset is too large!", functionName);
+        return WTF::nullopt;
+    }
     return GPUBufferBinding { bufferBinding.buffer.copyRef(), bufferBinding.offset, bufferBinding.size };
 }
 
@@ -71,7 +77,8 @@ static void setBufferOnEncoder(MTLArgumentEncoder *argumentEncoder, const GPUBuf
     ASSERT(argumentEncoder && bufferBinding.buffer->platformBuffer());
 
     BEGIN_BLOCK_OBJC_EXCEPTIONS;
-    [argumentEncoder setBuffer:bufferBinding.buffer->platformBuffer() offset:bufferBinding.offset atIndex:index];
+    // Bounds check when converting GPUBufferBinding ensures that NSUInteger cast of uint64_t offset is safe.
+    [argumentEncoder setBuffer:bufferBinding.buffer->platformBuffer() offset:static_cast<NSUInteger>(bufferBinding.offset) atIndex:index];
     END_BLOCK_OBJC_EXCEPTIONS;
 }
     
index 77e33f2..2a594b9 100644 (file)
@@ -45,17 +45,17 @@ static constexpr auto readOnlyFlags = OptionSet<GPUBufferUsage::Flags> { GPUBuff
 bool GPUBuffer::validateBufferUsage(const GPUDevice& device, OptionSet<GPUBufferUsage::Flags> usage)
 {
     if (!device.platformDevice()) {
-        LOG(WebGPU, "GPUBuffer::create(): Invalid GPUDevice!");
+        LOG(WebGPU, "GPUBuffer::tryCreate(): Invalid GPUDevice!");
         return false;
     }
 
     if (usage.containsAll({ GPUBufferUsage::Flags::MapWrite, GPUBufferUsage::Flags::MapRead })) {
-        LOG(WebGPU, "GPUBuffer::create(): Buffer cannot have both MAP_READ and MAP_WRITE usage!");
+        LOG(WebGPU, "GPUBuffer::tryCreate(): Buffer cannot have both MAP_READ and MAP_WRITE usage!");
         return false;
     }
 
     if (usage.containsAny(readOnlyFlags) && (usage & GPUBufferUsage::Flags::Storage)) {
-        LOG(WebGPU, "GPUBuffer::create(): Buffer cannot have both STORAGE and a read-only usage!");
+        LOG(WebGPU, "GPUBuffer::tryCreate(): Buffer cannot have both STORAGE and a read-only usage!");
         return false;
     }
 
@@ -64,6 +64,13 @@ bool GPUBuffer::validateBufferUsage(const GPUDevice& device, OptionSet<GPUBuffer
 
 RefPtr<GPUBuffer> GPUBuffer::tryCreate(Ref<GPUDevice>&& device, const GPUBufferDescriptor& descriptor)
 {
+    // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
+    NSUInteger size = 0;
+    if (!WTF::convertSafely<NSUInteger, uint64_t>(descriptor.size, size)) {
+        LOG(WebGPU, "GPUBuffer::tryCreate(): Buffer size is too large!");
+        return nullptr;
+    }
+
     auto usage = OptionSet<GPUBufferUsage::Flags>::fromRaw(descriptor.usage);
     if (!validateBufferUsage(device.get(), usage))
         return nullptr;
@@ -79,19 +86,19 @@ RefPtr<GPUBuffer> GPUBuffer::tryCreate(Ref<GPUDevice>&& device, const GPUBufferD
 
     BEGIN_BLOCK_OBJC_EXCEPTIONS;
 
-    mtlBuffer = adoptNS([device->platformDevice() newBufferWithLength:descriptor.size options:resourceOptions]);
+    mtlBuffer = adoptNS([device->platformDevice() newBufferWithLength:static_cast<NSUInteger>(descriptor.size) options:resourceOptions]);
 
     END_BLOCK_OBJC_EXCEPTIONS;
 
     if (!mtlBuffer) {
-        LOG(WebGPU, "GPUBuffer::create(): Unable to create MTLBuffer!");
+        LOG(WebGPU, "GPUBuffer::tryCreate(): Unable to create MTLBuffer!");
         return nullptr;
     }
 
-    return adoptRef(*new GPUBuffer(WTFMove(mtlBuffer), descriptor.size, usage, WTFMove(device)));
+    return adoptRef(*new GPUBuffer(WTFMove(mtlBuffer), size, usage, WTFMove(device)));
 }
 
-GPUBuffer::GPUBuffer(RetainPtr<MTLBuffer>&& buffer, uint64_t size, OptionSet<GPUBufferUsage::Flags> usage, Ref<GPUDevice>&& device)
+GPUBuffer::GPUBuffer(RetainPtr<MTLBuffer>&& buffer, size_t size, OptionSet<GPUBufferUsage::Flags> usage, Ref<GPUDevice>&& device)
     : m_platformBuffer(WTFMove(buffer))
     , m_device(WTFMove(device))
     , m_byteLength(size)
@@ -136,8 +143,8 @@ void GPUBuffer::setSubData(uint64_t offset, const JSC::ArrayBuffer& data)
         return;
     }
 #endif
-
-    auto subDataLength = checkedSum<uint64_t>(data.byteLength(), offset);
+    // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
+    auto subDataLength = checkedSum<NSUInteger>(data.byteLength(), offset);
     if (subDataLength.hasOverflowed() || subDataLength.unsafeGet() > m_byteLength) {
         LOG(WebGPU, "GPUBuffer::setSubData(): Invalid offset or data size!");
         return;
@@ -145,7 +152,7 @@ void GPUBuffer::setSubData(uint64_t offset, const JSC::ArrayBuffer& data)
 
     if (m_subDataBuffers.isEmpty()) {
         BEGIN_BLOCK_OBJC_EXCEPTIONS;
-        m_subDataBuffers.append(adoptNS([m_platformBuffer.get().device newBufferWithLength:m_byteLength options:MTLResourceCPUCacheModeDefaultCache]));
+        m_subDataBuffers.append(adoptNS([m_platformBuffer.get().device newBufferWithLength:static_cast<NSUInteger>(m_byteLength) options:MTLResourceCPUCacheModeDefaultCache]));
         END_BLOCK_OBJC_EXCEPTIONS;
     }
 
@@ -163,7 +170,7 @@ void GPUBuffer::setSubData(uint64_t offset, const JSC::ArrayBuffer& data)
     auto commandBuffer = retainPtr([queue commandBuffer]);
     auto blitEncoder = retainPtr([commandBuffer blitCommandEncoder]);
 
-    [blitEncoder copyFromBuffer:stagingMtlBuffer.get() sourceOffset:0 toBuffer:m_platformBuffer.get() destinationOffset:offset size:stagingMtlBuffer.get().length];
+    [blitEncoder copyFromBuffer:stagingMtlBuffer.get() sourceOffset:0 toBuffer:m_platformBuffer.get() destinationOffset:static_cast<NSUInteger>(offset) size:stagingMtlBuffer.get().length];
     [blitEncoder endEncoding];
 
     if (isMappable())
index 7ebd5bf..37cd45d 100644 (file)
@@ -107,8 +107,9 @@ void GPUCommandBuffer::copyBufferToBuffer(Ref<GPUBuffer>&& src, uint64_t srcOffs
     }
 #endif
 
-    auto srcLength = checkedSum<uint64_t>(size, srcOffset);
-    auto dstLength = checkedSum<uint64_t>(size, dstOffset);
+    // This call ensures that size, offset, and size + offset can safely fit in NSUInteger regardless of platform.
+    auto srcLength = checkedSum<NSUInteger>(size, srcOffset);
+    auto dstLength = checkedSum<NSUInteger>(size, dstOffset);
     if (srcLength.hasOverflowed() || dstLength.hasOverflowed()
         || srcLength.unsafeGet() > src->byteLength() || dstLength.unsafeGet() > dst->byteLength()) {
         LOG(WebGPU, "GPUCommandBuffer::copyBufferToBuffer(): Invalid offset or copy size!");
@@ -117,12 +118,13 @@ void GPUCommandBuffer::copyBufferToBuffer(Ref<GPUBuffer>&& src, uint64_t srcOffs
 
     BEGIN_BLOCK_OBJC_EXCEPTIONS;
 
+    // These casts are safe due to earlier checkedSum() checks.
     [blitEncoder()
         copyFromBuffer:src->platformBuffer()
-        sourceOffset:srcOffset
+        sourceOffset:static_cast<NSUInteger>(srcOffset)
         toBuffer:dst->platformBuffer()
-        destinationOffset:dstOffset
-        size:size];
+        destinationOffset:static_cast<NSUInteger>(dstOffset)
+        size:static_cast<NSUInteger>(size)];
 
     END_BLOCK_OBJC_EXCEPTIONS;
 
@@ -137,6 +139,13 @@ void GPUCommandBuffer::copyBufferToTexture(GPUBufferCopyView&& srcBuffer, GPUTex
         return;
     }
 
+    // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
+    NSUInteger sourceOffset = 0;
+    if (!WTF::convertSafely<NSUInteger, uint64_t>(srcBuffer.offset, sourceOffset)) {
+        LOG(WebGPU, "GPUCommandBuffer::copyBufferToTexture(): Source offset is too large!");
+        return;
+    }
+
     // FIXME: Add Metal validation.
 
     // GPUBufferCopyView::offset: The location must be aligned to the size of the destination texture's pixel format. The value must be a multiple of the destination texture's pixel size, in bytes.
@@ -153,7 +162,7 @@ void GPUCommandBuffer::copyBufferToTexture(GPUBufferCopyView&& srcBuffer, GPUTex
 
     [blitEncoder()
         copyFromBuffer:srcBuffer.buffer->platformBuffer()
-        sourceOffset:srcBuffer.offset
+        sourceOffset:sourceOffset
         sourceBytesPerRow:srcBuffer.rowPitch
         sourceBytesPerImage:srcBuffer.imageHeight
         sourceSize:MTLSizeMake(size.width, size.height, size.depth)
@@ -174,6 +183,12 @@ void GPUCommandBuffer::copyTextureToBuffer(GPUTextureCopyView&& srcTexture, GPUB
         LOG(WebGPU, "GPUCommandBuffer::copyTextureToBuffer(): Invalid operation!");
         return;
     }
+    // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
+    NSUInteger destinationOffset = 0;
+    if (!WTF::convertSafely<NSUInteger, uint64_t>(dstBuffer.offset, destinationOffset)) {
+        LOG(WebGPU, "GPUCommandBuffer::copyTextureToBuffer(): Destination offset is too large!");
+        return;
+    }
 
     // FIXME: Add Metal validation?
 
@@ -186,7 +201,7 @@ void GPUCommandBuffer::copyTextureToBuffer(GPUTextureCopyView&& srcTexture, GPUB
         sourceOrigin:MTLOriginMake(srcTexture.origin.x, srcTexture.origin.y, srcTexture.origin.z)
         sourceSize:MTLSizeMake(size.width, size.height, size.depth)
         toBuffer:dstBuffer.buffer->platformBuffer()
-        destinationOffset:dstBuffer.offset
+        destinationOffset:destinationOffset
         destinationBytesPerRow:dstBuffer.rowPitch
         destinationBytesPerImage:dstBuffer.imageHeight];
 
index 4c1d609..41a0406 100644 (file)
@@ -356,8 +356,12 @@ void GPURenderPassEncoder::drawIndexed(unsigned indexCount, unsigned instanceCou
     }
 
     auto indexByteSize = (m_pipeline->indexFormat() == GPUIndexFormat::Uint16) ? sizeof(uint16_t) : sizeof(uint32_t);
+
+    // This calculation cannot overflow as firstIndex is bounded to 32 bits, and indexByteSize to sizeof(uint32_t).
     uint64_t firstIndexOffset = firstIndex * indexByteSize;
-    auto totalOffset = checkedSum<uint64_t>(firstIndexOffset, m_indexBufferOffset);
+
+    // This call ensures that neither argument nor their sum will overflow NSUInteger.
+    auto totalOffset = checkedSum<NSUInteger>(firstIndexOffset, m_indexBufferOffset);
     if (totalOffset.hasOverflowed() || totalOffset >= m_indexBuffer->byteLength()) {
         LOG(WebGPU, "%s: Invalid firstIndex!", functionName);
         return;
index 0df1def..f3ea862 100644 (file)
@@ -36,6 +36,7 @@
 #import "WHLSLVertexBufferIndexCalculator.h"
 #import <Metal/Metal.h>
 #import <wtf/BlockObjCExceptions.h>
+#import <wtf/CheckedArithmetic.h>
 #import <wtf/OptionSet.h>
 #import <wtf/Optional.h>
 
@@ -339,10 +340,17 @@ static bool trySetInputStateForPipelineDescriptor(const char* const functionName
             LOG(WebGPU, "%s: Invalid inputSlot %u for vertex attribute %u!", functionName, attributes[i].inputSlot, location);
             return false;
         }
+        // MTLBuffer size (NSUInteger) is 32 bits on some platforms.
+        // FIXME: Ensure offset < buffer's stride + format's data size.
+        NSUInteger attributeOffset = 0;
+        if (!WTF::convertSafely<NSUInteger, uint64_t>(attributes[i].offset, attributeOffset)) {
+            LOG(WebGPU, "%s: Buffer offset for vertex attribute %u is too large!", functionName, location);
+            return false;
+        }
 
         auto mtlAttributeDesc = retainPtr([attributeArray objectAtIndexedSubscript:location]);
         [mtlAttributeDesc setFormat:mtlVertexFormatForGPUVertexFormat(attributes[i].format)];
-        [mtlAttributeDesc setOffset:attributes[i].offset]; // FIXME: After adding more vertex formats, ensure offset < buffer's stride + format's data size.
+        [mtlAttributeDesc setOffset:attributeOffset];
         [mtlAttributeDesc setBufferIndex:WHLSL::Metal::calculateVertexBufferIndex(attributes[i].inputSlot)];
     }
 
@@ -356,11 +364,16 @@ static bool trySetInputStateForPipelineDescriptor(const char* const functionName
             LOG(WebGPU, "%s: Invalid inputSlot %d for vertex buffer!", functionName, slot);
             return false;
         }
+        NSUInteger inputStride = 0;
+        if (!WTF::convertSafely<NSUInteger, uint64_t>(inputs[j].stride, inputStride)) {
+            LOG(WebGPU, "%s: Stride for vertex buffer slot %d is too large!", functionName, slot);
+            return false;
+        }
 
         auto convertedSlot = WHLSL::Metal::calculateVertexBufferIndex(slot);
         auto mtlLayoutDesc = retainPtr([layoutArray objectAtIndexedSubscript:convertedSlot]);
         [mtlLayoutDesc setStepFunction:mtlStepFunctionForGPUInputStepMode(inputs[j].stepMode)];
-        [mtlLayoutDesc setStride:inputs[j].stride];
+        [mtlLayoutDesc setStride:inputStride];
     }
 
     [mtlDescriptor setVertexDescriptor:mtlVertexDescriptor.get()];