Add more validations for input state
This change sets limit for stride in VertexInputDescriptor and
offset in VertexAttributeDescriptor, and adds validation code
for them.
It also uses existing descriptors to replace redundant definitions.
BUG=dawn:107
Change-Id: Ifbb07f48ec9a5baae8ae8d21865dc384670b759a
Reviewed-on: https://dawn-review.googlesource.com/c/4901
Commit-Queue: Yunchao He <yunchao.he@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/common/Constants.h b/src/common/Constants.h
index 238d0c2..7641502 100644
--- a/src/common/Constants.h
+++ b/src/common/Constants.h
@@ -22,7 +22,14 @@
// TODO(cwallez@chromium.org): investigate bindgroup limits
static constexpr uint32_t kMaxBindingsPerGroup = 16u;
static constexpr uint32_t kMaxVertexAttributes = 16u;
+// Vulkan has a standalone limit named maxVertexInputAttributeOffset (2047u at least) for vertex
+// attribute offset. The limit might be meaningless because Vulkan has another limit named
+// maxVertexInputBindingStride (2048u at least). We use maxVertexAttributeEnd (2048u) here to verify
+// vertex attribute offset, which equals to maxOffset + smallest size of vertex format (char). We
+// may use maxVertexInputStride instead in future.
+static constexpr uint32_t kMaxVertexAttributeEnd = 2048u;
static constexpr uint32_t kMaxVertexInputs = 16u;
+static constexpr uint32_t kMaxVertexInputStride = 2048u;
static constexpr uint32_t kNumStages = 3;
static constexpr uint32_t kMaxColorAttachments = 4u;
static constexpr uint32_t kTextureRowPitchAlignment = 256u;
diff --git a/src/dawn_native/InputState.cpp b/src/dawn_native/InputState.cpp
index 5272f0f..eb88ce7 100644
--- a/src/dawn_native/InputState.cpp
+++ b/src/dawn_native/InputState.cpp
@@ -95,7 +95,7 @@
return mAttributesSetMask;
}
- const InputStateBase::AttributeInfo& InputStateBase::GetAttribute(uint32_t location) const {
+ const VertexAttributeDescriptor& InputStateBase::GetAttribute(uint32_t location) const {
ASSERT(mAttributesSetMask[location]);
return mAttributeInfos[location];
}
@@ -104,7 +104,7 @@
return mInputsSetMask;
}
- const InputStateBase::InputInfo& InputStateBase::GetInput(uint32_t slot) const {
+ const VertexInputDescriptor& InputStateBase::GetInput(uint32_t slot) const {
ASSERT(mInputsSetMask[slot]);
return mInputInfos[slot];
}
@@ -135,16 +135,24 @@
HandleError("Binding slot out of bounds");
return;
}
+ // If attribute->offset is close to 0xFFFFFFFF, the validation below to add
+ // attribute->offset and VertexFormatSize(attribute->format) might overflow on a
+ // 32bit machine, then it can pass the validation incorrectly. We need to catch it.
+ if (attribute->offset >= kMaxVertexAttributeEnd) {
+ HandleError("Setting attribute offset out of bounds");
+ return;
+ }
+ if (attribute->offset + VertexFormatSize(attribute->format) > kMaxVertexAttributeEnd) {
+ HandleError("Setting attribute offset out of bounds");
+ return;
+ }
if (mAttributesSetMask[attribute->shaderLocation]) {
HandleError("Setting already set attribute");
return;
}
mAttributesSetMask.set(attribute->shaderLocation);
- auto& info = mAttributeInfos[attribute->shaderLocation];
- info.inputSlot = attribute->inputSlot;
- info.offset = attribute->offset;
- info.format = attribute->format;
+ mAttributeInfos[attribute->shaderLocation] = *attribute;
}
void InputStateBuilder::SetInput(const VertexInputDescriptor* input) {
@@ -152,15 +160,17 @@
HandleError("Setting input out of bounds");
return;
}
+ if (input->stride > kMaxVertexInputStride) {
+ HandleError("Setting input stride out of bounds");
+ return;
+ }
if (mInputsSetMask[input->inputSlot]) {
HandleError("Setting already set input");
return;
}
mInputsSetMask.set(input->inputSlot);
- auto& info = mInputInfos[input->inputSlot];
- info.stride = input->stride;
- info.stepMode = input->stepMode;
+ mInputInfos[input->inputSlot] = *input;
}
} // namespace dawn_native
diff --git a/src/dawn_native/InputState.h b/src/dawn_native/InputState.h
index 5d5300a..adf5d98 100644
--- a/src/dawn_native/InputState.h
+++ b/src/dawn_native/InputState.h
@@ -36,27 +36,16 @@
public:
InputStateBase(InputStateBuilder* builder);
- struct AttributeInfo {
- uint32_t inputSlot;
- dawn::VertexFormat format;
- uint32_t offset;
- };
-
- struct InputInfo {
- uint32_t stride;
- dawn::InputStepMode stepMode;
- };
-
const std::bitset<kMaxVertexAttributes>& GetAttributesSetMask() const;
- const AttributeInfo& GetAttribute(uint32_t location) const;
+ const VertexAttributeDescriptor& GetAttribute(uint32_t location) const;
const std::bitset<kMaxVertexInputs>& GetInputsSetMask() const;
- const InputInfo& GetInput(uint32_t slot) const;
+ const VertexInputDescriptor& GetInput(uint32_t slot) const;
private:
std::bitset<kMaxVertexAttributes> mAttributesSetMask;
- std::array<AttributeInfo, kMaxVertexAttributes> mAttributeInfos;
+ std::array<VertexAttributeDescriptor, kMaxVertexAttributes> mAttributeInfos;
std::bitset<kMaxVertexInputs> mInputsSetMask;
- std::array<InputInfo, kMaxVertexInputs> mInputInfos;
+ std::array<VertexInputDescriptor, kMaxVertexInputs> mInputInfos;
};
class InputStateBuilder : public Builder<InputStateBase> {
@@ -73,9 +62,9 @@
InputStateBase* GetResultImpl() override;
std::bitset<kMaxVertexAttributes> mAttributesSetMask;
- std::array<InputStateBase::AttributeInfo, kMaxVertexAttributes> mAttributeInfos;
+ std::array<VertexAttributeDescriptor, kMaxVertexAttributes> mAttributeInfos;
std::bitset<kMaxVertexInputs> mInputsSetMask;
- std::array<InputStateBase::InputInfo, kMaxVertexInputs> mInputInfos;
+ std::array<VertexInputDescriptor, kMaxVertexInputs> mInputInfos;
};
} // namespace dawn_native
diff --git a/src/dawn_native/d3d12/InputStateD3D12.cpp b/src/dawn_native/d3d12/InputStateD3D12.cpp
index 9178423..edeb4c0 100644
--- a/src/dawn_native/d3d12/InputStateD3D12.cpp
+++ b/src/dawn_native/d3d12/InputStateD3D12.cpp
@@ -71,7 +71,7 @@
D3D12_INPUT_ELEMENT_DESC& inputElementDescriptor = mInputElementDescriptors[count++];
- const AttributeInfo& attribute = GetAttribute(i);
+ const VertexAttributeDescriptor& attribute = GetAttribute(i);
// If the HLSL semantic is TEXCOORDN the SemanticName should be "TEXCOORD" and the
// SemanticIndex N
@@ -80,7 +80,7 @@
inputElementDescriptor.Format = VertexFormatType(attribute.format);
inputElementDescriptor.InputSlot = attribute.inputSlot;
- const InputInfo& input = GetInput(attribute.inputSlot);
+ const VertexInputDescriptor& input = GetInput(attribute.inputSlot);
inputElementDescriptor.AlignedByteOffset = attribute.offset;
inputElementDescriptor.InputSlotClass = InputStepModeFunction(input.stepMode);
diff --git a/src/dawn_native/metal/InputStateMTL.mm b/src/dawn_native/metal/InputStateMTL.mm
index d21b2e7..02a0c8a 100644
--- a/src/dawn_native/metal/InputStateMTL.mm
+++ b/src/dawn_native/metal/InputStateMTL.mm
@@ -66,7 +66,7 @@
if (!attributesSetMask[i]) {
continue;
}
- const AttributeInfo& info = GetAttribute(i);
+ const VertexAttributeDescriptor& info = GetAttribute(i);
auto attribDesc = [MTLVertexAttributeDescriptor new];
attribDesc.format = VertexFormatType(info.format);
@@ -77,7 +77,7 @@
}
for (uint32_t i : IterateBitSet(GetInputsSetMask())) {
- const InputInfo& info = GetInput(i);
+ const VertexInputDescriptor& info = GetInput(i);
auto layoutDesc = [MTLVertexBufferLayoutDescriptor new];
if (info.stride == 0) {
diff --git a/src/tests/unittests/validation/InputStateValidationTests.cpp b/src/tests/unittests/validation/InputStateValidationTests.cpp
index 724a061..688cd2f 100644
--- a/src/tests/unittests/validation/InputStateValidationTests.cpp
+++ b/src/tests/unittests/validation/InputStateValidationTests.cpp
@@ -146,9 +146,9 @@
.GetResult();
}
-// Check out of bounds condition on SetInput
-TEST_F(InputStateTest, SetInputOutOfBounds) {
- // Control case, setting last input
+// Check out of bounds condition on input slot
+TEST_F(InputStateTest, SetInputSlotOutOfBounds) {
+ // Control case, setting last input slot
dawn::VertexInputDescriptor input;
input.inputSlot = kMaxVertexInputs - 1;
input.stride = 0;
@@ -156,11 +156,25 @@
AssertWillBeSuccess(device.CreateInputStateBuilder()).SetInput(&input).GetResult();
- // Test OOB
+ // Test input slot OOB
input.inputSlot = kMaxVertexInputs;
AssertWillBeError(device.CreateInputStateBuilder()).SetInput(&input).GetResult();
}
+// Check out of bounds condition on input stride
+TEST_F(InputStateTest, SetInputStrideOutOfBounds) {
+ // Control case, setting max input stride
+ dawn::VertexInputDescriptor input;
+ input.inputSlot = 0;
+ input.stride = kMaxVertexInputStride;
+ input.stepMode = dawn::InputStepMode::Vertex;
+ AssertWillBeSuccess(device.CreateInputStateBuilder()).SetInput(&input).GetResult();
+
+ // Test input stride OOB
+ input.stride = kMaxVertexInputStride + 1;
+ AssertWillBeError(device.CreateInputStateBuilder()).SetInput(&input).GetResult();
+}
+
// Test that we cannot set an already set attribute
TEST_F(InputStateTest, AlreadySetAttribute) {
// Control case, setting last attribute
@@ -183,9 +197,9 @@
.GetResult();
}
-// Check out of bounds condition on SetAttribute
-TEST_F(InputStateTest, SetAttributeOutOfBounds) {
- // Control case, setting last attribute
+// Check out of bounds condition on attribute shader location
+TEST_F(InputStateTest, SetAttributeLocationOutOfBounds) {
+ // Control case, setting last attribute shader location
dawn::VertexAttributeDescriptor attribute;
attribute.shaderLocation = kMaxVertexAttributes - 1;
attribute.inputSlot = 0;
@@ -197,7 +211,7 @@
.SetAttribute(&attribute)
.GetResult();
- // Test OOB
+ // Test attribute location OOB
attribute.shaderLocation = kMaxVertexAttributes;
AssertWillBeError(device.CreateInputStateBuilder())
.SetInput(&kBaseInput)
@@ -205,6 +219,40 @@
.GetResult();
}
+// Check attribute offset out of bounds
+TEST_F(InputStateTest, SetAttributeOffsetOutOfBounds) {
+ // Control case, setting max attribute offset for FloatR32 vertex format
+ dawn::VertexAttributeDescriptor attribute;
+ attribute.shaderLocation = 0;
+ attribute.inputSlot = 0;
+ attribute.offset = kMaxVertexAttributeEnd - sizeof(dawn::VertexFormat::FloatR32);
+ attribute.format = dawn::VertexFormat::FloatR32;
+ AssertWillBeSuccess(device.CreateInputStateBuilder())
+ .SetInput(&kBaseInput)
+ .SetAttribute(&attribute)
+ .GetResult();
+
+ // Test attribute offset out of bounds
+ attribute.offset = kMaxVertexAttributeEnd - 1;
+ AssertWillBeError(device.CreateInputStateBuilder())
+ .SetInput(&kBaseInput)
+ .SetAttribute(&attribute)
+ .GetResult();
+}
+
+// Check attribute offset overflow
+TEST_F(InputStateTest, SetAttributeOffsetOverflow) {
+ dawn::VertexAttributeDescriptor attribute;
+ attribute.shaderLocation = 0;
+ attribute.inputSlot = 0;
+ attribute.offset = std::numeric_limits<uint32_t>::max();
+ attribute.format = dawn::VertexFormat::FloatR32;
+ AssertWillBeError(device.CreateInputStateBuilder())
+ .SetInput(&kBaseInput)
+ .SetAttribute(&attribute)
+ .GetResult();
+}
+
// Check that all attributes must be backed by an input
TEST_F(InputStateTest, RequireInputForAttribute) {
// Control case
