From f603903da71e4e84b705d047be6a63faadb4c6b0 Mon Sep 17 00:00:00 2001 From: Bryan Bernhart Date: Fri, 18 Oct 2019 16:19:00 +0000 Subject: [PATCH] Replace size_t with uint64_t in ringbuffer. Adds overflow check in RingBufferAllocator + unit-test. Also, update clients to use uint64_t to avoid casts or narrowing. BUG=dawn:233 Change-Id: I652e3142407006d082491add600371f95d44741a Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/12380 Reviewed-by: Austin Eng Reviewed-by: Corentin Wallez Commit-Queue: Bryan Bernhart --- src/dawn_native/DynamicUploader.cpp | 8 +++--- src/dawn_native/DynamicUploader.h | 8 +++--- src/dawn_native/RingBufferAllocator.cpp | 18 ++++++++----- src/dawn_native/RingBufferAllocator.h | 24 ++++++++--------- .../d3d12/DescriptorHeapAllocator.cpp | 11 ++++---- .../d3d12/DescriptorHeapAllocator.h | 4 +-- .../unittests/RingBufferAllocatorTests.cpp | 27 +++++++++++++------ 7 files changed, 58 insertions(+), 42 deletions(-) diff --git a/src/dawn_native/DynamicUploader.cpp b/src/dawn_native/DynamicUploader.cpp index 876b789668..9996ee1edf 100644 --- a/src/dawn_native/DynamicUploader.cpp +++ b/src/dawn_native/DynamicUploader.cpp @@ -18,7 +18,7 @@ namespace dawn_native { - DynamicUploader::DynamicUploader(DeviceBase* device, size_t size) : mDevice(device) { + DynamicUploader::DynamicUploader(DeviceBase* device, uint64_t size) : mDevice(device) { mRingBuffers.emplace_back( std::unique_ptr(new RingBuffer{nullptr, RingBufferAllocator(size)})); } @@ -28,7 +28,7 @@ namespace dawn_native { mDevice->GetPendingCommandSerial()); } - ResultOrError DynamicUploader::Allocate(size_t allocationSize, Serial serial) { + ResultOrError DynamicUploader::Allocate(uint64_t allocationSize, Serial serial) { // Note: Validation ensures size is already aligned. // First-fit: find next smallest buffer large enough to satisfy the allocation request. RingBuffer* targetRingBuffer = mRingBuffers.back().get(); @@ -36,7 +36,7 @@ namespace dawn_native { const RingBufferAllocator& ringBufferAllocator = ringBuffer->mAllocator; // Prevent overflow. ASSERT(ringBufferAllocator.GetSize() >= ringBufferAllocator.GetUsedSize()); - const size_t remainingSize = + const uint64_t remainingSize = ringBufferAllocator.GetSize() - ringBufferAllocator.GetUsedSize(); if (allocationSize <= remainingSize) { targetRingBuffer = ringBuffer.get(); @@ -44,7 +44,7 @@ namespace dawn_native { } } - size_t startOffset = RingBufferAllocator::kInvalidOffset; + uint64_t startOffset = RingBufferAllocator::kInvalidOffset; if (targetRingBuffer != nullptr) { startOffset = targetRingBuffer->mAllocator.Allocate(allocationSize, serial); } diff --git a/src/dawn_native/DynamicUploader.h b/src/dawn_native/DynamicUploader.h index f0d4510f15..068eebd060 100644 --- a/src/dawn_native/DynamicUploader.h +++ b/src/dawn_native/DynamicUploader.h @@ -25,13 +25,13 @@ namespace dawn_native { struct UploadHandle { uint8_t* mappedBuffer = nullptr; - size_t startOffset = 0; + uint64_t startOffset = 0; StagingBufferBase* stagingBuffer = nullptr; }; class DynamicUploader { public: - DynamicUploader(DeviceBase* device, size_t size = kBaseUploadBufferSize); + DynamicUploader(DeviceBase* device, uint64_t size = kBaseUploadBufferSize); ~DynamicUploader() = default; // We add functions to Release StagingBuffers to the DynamicUploader as there's @@ -40,12 +40,12 @@ namespace dawn_native { // implemented. void ReleaseStagingBuffer(std::unique_ptr stagingBuffer); - ResultOrError Allocate(size_t allocationSize, Serial serial); + ResultOrError Allocate(uint64_t allocationSize, Serial serial); void Deallocate(Serial lastCompletedSerial); private: // TODO(bryan.bernhart@intel.com): Figure out this value. - static constexpr size_t kBaseUploadBufferSize = 64000; + static constexpr uint64_t kBaseUploadBufferSize = 64000; struct RingBuffer { std::unique_ptr mStagingBuffer; diff --git a/src/dawn_native/RingBufferAllocator.cpp b/src/dawn_native/RingBufferAllocator.cpp index 6cb94b7048..6c3eef0ce0 100644 --- a/src/dawn_native/RingBufferAllocator.cpp +++ b/src/dawn_native/RingBufferAllocator.cpp @@ -29,7 +29,7 @@ // TODO(bryan.bernhart@intel.com): Follow-up with ringbuffer optimization. namespace dawn_native { - RingBufferAllocator::RingBufferAllocator(size_t maxSize) : mMaxBlockSize(maxSize) { + RingBufferAllocator::RingBufferAllocator(uint64_t maxSize) : mMaxBlockSize(maxSize) { } void RingBufferAllocator::Deallocate(Serial lastCompletedSerial) { @@ -43,11 +43,11 @@ namespace dawn_native { mInflightRequests.ClearUpTo(lastCompletedSerial); } - size_t RingBufferAllocator::GetSize() const { + uint64_t RingBufferAllocator::GetSize() const { return mMaxBlockSize; } - size_t RingBufferAllocator::GetUsedSize() const { + uint64_t RingBufferAllocator::GetUsedSize() const { return mUsedSize; } @@ -62,7 +62,7 @@ namespace dawn_native { // queue, which identifies an existing (or new) frames-worth of resources. Internally, the // ring-buffer maintains offsets of 3 "memory" states: Free, Reclaimed, and Used. This is done // in FIFO order as older frames would free resources before newer ones. - size_t RingBufferAllocator::Allocate(size_t allocationSize, Serial serial) { + uint64_t RingBufferAllocator::Allocate(uint64_t allocationSize, Serial serial) { // Check if the buffer is full by comparing the used size. // If the buffer is not split where waste occurs (e.g. cannot fit new sub-alloc in front), a // subsequent sub-alloc could fail where the used size was previously adjusted to include @@ -71,7 +71,13 @@ namespace dawn_native { return kInvalidOffset; } - size_t startOffset = kInvalidOffset; + // Ensure adding allocationSize does not overflow. + const uint64_t remainingSize = (mMaxBlockSize - mUsedSize); + if (allocationSize > remainingSize) { + return kInvalidOffset; + } + + uint64_t startOffset = kInvalidOffset; // Check if the buffer is NOT split (i.e sub-alloc on ends) if (mUsedStartOffset <= mUsedEndOffset) { @@ -86,7 +92,7 @@ namespace dawn_native { } else if (allocationSize <= mUsedStartOffset) { // Try to sub-alloc at front. // Count the space at the end so that a subsequent // sub-alloc cannot not succeed when the buffer is full. - const size_t requestSize = (mMaxBlockSize - mUsedEndOffset) + allocationSize; + const uint64_t requestSize = (mMaxBlockSize - mUsedEndOffset) + allocationSize; startOffset = 0; mUsedEndOffset = allocationSize; diff --git a/src/dawn_native/RingBufferAllocator.h b/src/dawn_native/RingBufferAllocator.h index 60ee6395c4..e437632d7c 100644 --- a/src/dawn_native/RingBufferAllocator.h +++ b/src/dawn_native/RingBufferAllocator.h @@ -26,32 +26,32 @@ namespace dawn_native { class RingBufferAllocator { public: RingBufferAllocator() = default; - RingBufferAllocator(size_t maxSize); + RingBufferAllocator(uint64_t maxSize); ~RingBufferAllocator() = default; - size_t Allocate(size_t allocationSize, Serial serial); + uint64_t Allocate(uint64_t allocationSize, Serial serial); void Deallocate(Serial lastCompletedSerial); - size_t GetSize() const; + uint64_t GetSize() const; bool Empty() const; - size_t GetUsedSize() const; + uint64_t GetUsedSize() const; - static constexpr size_t kInvalidOffset = std::numeric_limits::max(); + static constexpr uint64_t kInvalidOffset = std::numeric_limits::max(); private: struct Request { - size_t endOffset; - size_t size; + uint64_t endOffset; + uint64_t size; }; SerialQueue mInflightRequests; // Queue of the recorded sub-alloc requests (e.g. // frame of resources). - size_t mUsedEndOffset = 0; // Tail of used sub-alloc requests (in bytes). - size_t mUsedStartOffset = 0; // Head of used sub-alloc requests (in bytes). - size_t mMaxBlockSize = 0; // Max size of the ring buffer (in bytes). - size_t mUsedSize = 0; // Size of the sub-alloc requests (in bytes) of the ring buffer. - size_t mCurrentRequestSize = + uint64_t mUsedEndOffset = 0; // Tail of used sub-alloc requests (in bytes). + uint64_t mUsedStartOffset = 0; // Head of used sub-alloc requests (in bytes). + uint64_t mMaxBlockSize = 0; // Max size of the ring buffer (in bytes). + uint64_t mUsedSize = 0; // Size of the sub-alloc requests (in bytes) of the ring buffer. + uint64_t mCurrentRequestSize = 0; // Size of the sub-alloc requests (in bytes) of the current serial. }; } // namespace dawn_native diff --git a/src/dawn_native/d3d12/DescriptorHeapAllocator.cpp b/src/dawn_native/d3d12/DescriptorHeapAllocator.cpp index a69641a152..19804529cb 100644 --- a/src/dawn_native/d3d12/DescriptorHeapAllocator.cpp +++ b/src/dawn_native/d3d12/DescriptorHeapAllocator.cpp @@ -25,7 +25,7 @@ namespace dawn_native { namespace d3d12 { DescriptorHeapHandle::DescriptorHeapHandle(ComPtr descriptorHeap, uint32_t sizeIncrement, - uint32_t offset) + uint64_t offset) : mDescriptorHeap(descriptorHeap), mSizeIncrement(sizeIncrement), mOffset(offset) { } @@ -68,12 +68,11 @@ namespace dawn_native { namespace d3d12 { DescriptorHeapInfo* heapInfo, D3D12_DESCRIPTOR_HEAP_FLAGS flags) { const Serial pendingSerial = mDevice->GetPendingCommandSerial(); - size_t startOffset = (heapInfo->heap == nullptr) - ? RingBufferAllocator::kInvalidOffset - : heapInfo->allocator.Allocate(count, pendingSerial); + uint64_t startOffset = (heapInfo->heap == nullptr) + ? RingBufferAllocator::kInvalidOffset + : heapInfo->allocator.Allocate(count, pendingSerial); if (startOffset != RingBufferAllocator::kInvalidOffset) { - return DescriptorHeapHandle{heapInfo->heap, mSizeIncrements[type], - static_cast(startOffset)}; + return DescriptorHeapHandle{heapInfo->heap, mSizeIncrements[type], startOffset}; } // If the pool has no more space, replace the pool with a new one of the specified size diff --git a/src/dawn_native/d3d12/DescriptorHeapAllocator.h b/src/dawn_native/d3d12/DescriptorHeapAllocator.h index e4949a68cd..bcb6ff5f01 100644 --- a/src/dawn_native/d3d12/DescriptorHeapAllocator.h +++ b/src/dawn_native/d3d12/DescriptorHeapAllocator.h @@ -33,7 +33,7 @@ namespace dawn_native { namespace d3d12 { DescriptorHeapHandle(); DescriptorHeapHandle(ComPtr descriptorHeap, uint32_t sizeIncrement, - uint32_t offset); + uint64_t offset); ID3D12DescriptorHeap* Get() const; D3D12_CPU_DESCRIPTOR_HANDLE GetCPUHandle(uint32_t index) const; @@ -42,7 +42,7 @@ namespace dawn_native { namespace d3d12 { private: ComPtr mDescriptorHeap; uint32_t mSizeIncrement; - uint32_t mOffset; + uint64_t mOffset; }; class DescriptorHeapAllocator { diff --git a/src/tests/unittests/RingBufferAllocatorTests.cpp b/src/tests/unittests/RingBufferAllocatorTests.cpp index 2455e8d6d4..a3e5a62134 100644 --- a/src/tests/unittests/RingBufferAllocatorTests.cpp +++ b/src/tests/unittests/RingBufferAllocatorTests.cpp @@ -18,11 +18,11 @@ using namespace dawn_native; -constexpr size_t RingBufferAllocator::kInvalidOffset; +constexpr uint64_t RingBufferAllocator::kInvalidOffset; // Number of basic tests for Ringbuffer TEST(RingBufferAllocatorTests, BasicTest) { - constexpr size_t sizeInBytes = 64000; + constexpr uint64_t sizeInBytes = 64000; RingBufferAllocator allocator(sizeInBytes); // Ensure no requests exist on empty buffer. @@ -43,8 +43,8 @@ TEST(RingBufferAllocatorTests, BasicTest) { // Tests that several ringbuffer allocations do not fail. TEST(RingBufferAllocatorTests, RingBufferManyAlloc) { - constexpr size_t maxNumOfFrames = 64000; - constexpr size_t frameSizeInBytes = 4; + constexpr uint64_t maxNumOfFrames = 64000; + constexpr uint64_t frameSizeInBytes = 4; RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes); @@ -57,8 +57,8 @@ TEST(RingBufferAllocatorTests, RingBufferManyAlloc) { // Tests ringbuffer sub-allocations of the same serial are correctly tracked. TEST(RingBufferAllocatorTests, AllocInSameFrame) { - constexpr size_t maxNumOfFrames = 3; - constexpr size_t frameSizeInBytes = 4; + constexpr uint64_t maxNumOfFrames = 3; + constexpr uint64_t frameSizeInBytes = 4; RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes); @@ -87,8 +87,8 @@ TEST(RingBufferAllocatorTests, AllocInSameFrame) { // Tests ringbuffer sub-allocation at various offsets. TEST(RingBufferAllocatorTests, RingBufferSubAlloc) { - constexpr size_t maxNumOfFrames = 10; - constexpr size_t frameSizeInBytes = 4; + constexpr uint64_t maxNumOfFrames = 10; + constexpr uint64_t frameSizeInBytes = 4; RingBufferAllocator allocator(maxNumOfFrames * frameSizeInBytes); @@ -164,3 +164,14 @@ TEST(RingBufferAllocatorTests, RingBufferSubAlloc) { EXPECT_TRUE(allocator.Empty()); } + +// Checks if ringbuffer sub-allocation does not overflow. +TEST(RingBufferAllocatorTests, RingBufferOverflow) { + Serial serial = 1; + + RingBufferAllocator allocator(std::numeric_limits::max()); + + ASSERT_EQ(allocator.Allocate(1, serial), 0u); + ASSERT_EQ(allocator.Allocate(std::numeric_limits::max(), serial + 1), + RingBufferAllocator::kInvalidOffset); +} \ No newline at end of file