From 2b82eb290220668cc1459a591558d20d03614800 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Fri, 7 Jun 2019 11:05:37 +0000 Subject: [PATCH] Fix incorrect ASSERT in vertex validation. Also adds a test that would have fired the ASSERT. BUG=dawn:80 BUG=dawn:107 Change-Id: I56cdbc91956465c8941b45bb5e9da4c27da301ae Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/7840 Commit-Queue: Corentin Wallez Reviewed-by: Yunchao He Reviewed-by: Kai Ninomiya --- src/dawn_native/RenderPipeline.cpp | 18 +++++++++++++----- .../validation/VertexInputValidationTests.cpp | 15 +++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp index 3e37f7b1f8..a0b97bc867 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -34,14 +34,22 @@ namespace dawn_native { if (attribute->shaderLocation >= kMaxVertexAttributes) { return DAWN_VALIDATION_ERROR("Setting attribute out of bounds"); } + + // No underflow is possible because the max vertex format size is smaller than + // kMaxVertexAttributeEnd. ASSERT(kMaxVertexAttributeEnd >= VertexFormatSize(attribute->format)); - ASSERT(vertexBufferStride == 0 || - vertexBufferStride >= VertexFormatSize(attribute->format)); - if (attribute->offset > kMaxVertexAttributeEnd - VertexFormatSize(attribute->format) || - (vertexBufferStride > 0 && - attribute->offset + VertexFormatSize(attribute->format) > vertexBufferStride)) { + if (attribute->offset > kMaxVertexAttributeEnd - VertexFormatSize(attribute->format)) { return DAWN_VALIDATION_ERROR("Setting attribute offset out of bounds"); } + + // No overflow is possible because the offset is already validated to be less + // than kMaxVertexAttributeEnd. + ASSERT(attribute->offset < kMaxVertexAttributeEnd); + if (vertexBufferStride > 0 && + attribute->offset + VertexFormatSize(attribute->format) > vertexBufferStride) { + return DAWN_VALIDATION_ERROR("Setting attribute offset out of bounds"); + } + if ((*attributesSetMask)[attribute->shaderLocation]) { return DAWN_VALIDATION_ERROR("Setting already set attribute"); } diff --git a/src/tests/unittests/validation/VertexInputValidationTests.cpp b/src/tests/unittests/validation/VertexInputValidationTests.cpp index f803023ff8..f855cedcd5 100644 --- a/src/tests/unittests/validation/VertexInputValidationTests.cpp +++ b/src/tests/unittests/validation/VertexInputValidationTests.cpp @@ -363,3 +363,18 @@ TEST_F(VertexInputTest, SetAttributeOffsetOverflow) { } )"); } + +// Check for some potential underflow in the vertex input validation +TEST_F(VertexInputTest, VertexFormatLargerThanNonZeroStride) { + utils::ComboVertexInputDescriptor state; + state.bufferCount = 1; + state.cBuffers[0].stride = 4; + state.cBuffers[0].attributeCount = 1; + state.cAttributes[0].format = dawn::VertexFormat::Float4; + CreatePipeline(false, state, R"( + #version 450 + void main() { + gl_Position = vec4(0.0); + } + )"); +}