From 91f33503fc995da83bbadf6b2a55b47364fa44e9 Mon Sep 17 00:00:00 2001 From: Steven Perron Date: Tue, 6 Nov 2018 11:30:19 -0500 Subject: [PATCH] Validate the id bound. (#2031) * Validate the id bound. Validates that the id bound for the module is not larger than the max id bound. Also adds an option to set the max id bound. Allows the optimizer option to set the max id bound to also set the id bound for the validation run done by the optimizer. Fixes #2030. --- include/spirv-tools/libspirv.h | 1 + source/spirv_validator_options.cpp | 3 ++ source/spirv_validator_options.h | 1 + source/val/validate.cpp | 6 +++ test/val/val_limits_test.cpp | 64 ++++++++++++++++++++++++++++++ tools/opt/opt.cpp | 8 ++-- tools/val/val.cpp | 1 + 7 files changed, 81 insertions(+), 3 deletions(-) diff --git a/include/spirv-tools/libspirv.h b/include/spirv-tools/libspirv.h index e202f6fff..050fecc86 100644 --- a/include/spirv-tools/libspirv.h +++ b/include/spirv-tools/libspirv.h @@ -435,6 +435,7 @@ typedef enum { spv_validator_limit_max_function_args, spv_validator_limit_max_control_flow_nesting_depth, spv_validator_limit_max_access_chain_indexes, + spv_validator_limit_max_id_bound, } spv_validator_limit; // Returns a string describing the given SPIR-V target environment. diff --git a/source/spirv_validator_options.cpp b/source/spirv_validator_options.cpp index 0c0625364..6d9afab7e 100644 --- a/source/spirv_validator_options.cpp +++ b/source/spirv_validator_options.cpp @@ -37,6 +37,8 @@ bool spvParseUniversalLimitsOptions(const char* s, spv_validator_limit* type) { *type = spv_validator_limit_max_control_flow_nesting_depth; } else if (match("--max-access-chain-indexes")) { *type = spv_validator_limit_max_access_chain_indexes; + } else if (match("--max-id-bound")) { + *type = spv_validator_limit_max_id_bound; } else { // The command line option for this validator limit has not been added. // Therefore we return false. @@ -73,6 +75,7 @@ void spvValidatorOptionsSetUniversalLimit(spv_validator_options options, max_control_flow_nesting_depth) LIMIT(spv_validator_limit_max_access_chain_indexes, max_access_chain_indexes) + LIMIT(spv_validator_limit_max_id_bound, max_id_bound) #undef LIMIT } } diff --git a/source/spirv_validator_options.h b/source/spirv_validator_options.h index d264a7e0b..ba1c8daeb 100644 --- a/source/spirv_validator_options.h +++ b/source/spirv_validator_options.h @@ -32,6 +32,7 @@ struct validator_universal_limits_t { uint32_t max_function_args{255}; uint32_t max_control_flow_nesting_depth{1023}; uint32_t max_access_chain_indexes{255}; + uint32_t max_id_bound{0x3FFFFF}; }; // Manages command line options passed to the SPIR-V Validator. New struct diff --git a/source/val/validate.cpp b/source/val/validate.cpp index 98088c512..22367909c 100644 --- a/source/val/validate.cpp +++ b/source/val/validate.cpp @@ -226,6 +226,12 @@ spv_result_t ValidateBinaryUsingContextAndValidationState( << spvTargetEnvDescription(context.target_env) << "."; } + if (header.bound > vstate->options()->universal_limits_.max_id_bound) { + return DiagnosticStream(position, context.consumer, "", + SPV_ERROR_INVALID_BINARY) + << "Invalid SPIR-V. The id bound is larger than the max id bound " + << vstate->options()->universal_limits_.max_id_bound << "."; + } // Look for OpExtension instructions and register extensions. spvBinaryParse(&context, vstate, words, num_words, /* parsed_header = */ nullptr, ProcessExtensions, diff --git a/test/val/val_limits_test.cpp b/test/val/val_limits_test.cpp index 55bf1e5f1..61ab15c88 100644 --- a/test/val/val_limits_test.cpp +++ b/test/val/val_limits_test.cpp @@ -79,6 +79,70 @@ TEST_F(ValidateLimits, IdEqualToBoundBad) { HasSubstr("Result '64' must be less than the ID bound '64'.")); } +TEST_F(ValidateLimits, IdBoundTooBigDeaultLimit) { + std::string str = header; + + CompileSuccessfully(str); + + // The largest ID used in this program is 64. Let's overwrite the ID bound in + // the header to be 64. This should result in an error because all IDs must + // satisfy: 0 < id < bound. + OverwriteAssembledBinary(3, 0x4FFFFF); + + ASSERT_EQ(SPV_ERROR_INVALID_BINARY, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Invalid SPIR-V. The id bound is larger than the max " + "id bound 4194303.")); +} + +TEST_F(ValidateLimits, IdBoundAtSetLimit) { + std::string str = header; + + CompileSuccessfully(str); + + // The largest ID used in this program is 64. Let's overwrite the ID bound in + // the header to be 64. This should result in an error because all IDs must + // satisfy: 0 < id < bound. + uint32_t id_bound = 0x4FFFFF; + + OverwriteAssembledBinary(3, id_bound); + getValidatorOptions()->universal_limits_.max_id_bound = id_bound; + + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()); +} + +TEST_F(ValidateLimits, IdBoundJustAboveSetLimit) { + std::string str = header; + + CompileSuccessfully(str); + + // The largest ID used in this program is 64. Let's overwrite the ID bound in + // the header to be 64. This should result in an error because all IDs must + // satisfy: 0 < id < bound. + uint32_t id_bound = 5242878; + + OverwriteAssembledBinary(3, id_bound); + getValidatorOptions()->universal_limits_.max_id_bound = id_bound - 1; + + ASSERT_EQ(SPV_ERROR_INVALID_BINARY, ValidateInstructions()); + EXPECT_THAT(getDiagnosticString(), + HasSubstr("Invalid SPIR-V. The id bound is larger than the max " + "id bound 5242877.")); +} + +TEST_F(ValidateLimits, IdBoundAtInMaxLimit) { + std::string str = header; + + CompileSuccessfully(str); + + uint32_t id_bound = std::numeric_limits::max(); + + OverwriteAssembledBinary(3, id_bound); + getValidatorOptions()->universal_limits_.max_id_bound = id_bound; + + ASSERT_EQ(SPV_SUCCESS, ValidateInstructions()); +} + TEST_F(ValidateLimits, StructNumMembersGood) { std::ostringstream spirv; spirv << header << R"( diff --git a/tools/opt/opt.cpp b/tools/opt/opt.cpp index e024a1b69..119bad9c5 100644 --- a/tools/opt/opt.cpp +++ b/tools/opt/opt.cpp @@ -557,6 +557,8 @@ OptStatus ParseFlags(int argc, const char** argv, return {OPT_STOP, 1}; } optimizer_options->set_max_id_bound(max_id_bound); + validator_options->SetUniversalLimit(spv_validator_limit_max_id_bound, + max_id_bound); } else { // Some passes used to accept the form '--pass arg', canonicalize them // to '--pass=arg'. @@ -593,16 +595,16 @@ int main(int argc, const char** argv) { const char* out_file = nullptr; spv_target_env target_env = kDefaultEnvironment; - spvtools::ValidatorOptions validator_options; - spvtools::OptimizerOptions optimizer_options; - optimizer_options.set_validator_options(validator_options); spvtools::Optimizer optimizer(target_env); optimizer.SetMessageConsumer(spvtools::utils::CLIMessageConsumer); + spvtools::ValidatorOptions validator_options; + spvtools::OptimizerOptions optimizer_options; OptStatus status = ParseFlags(argc, argv, &optimizer, &in_file, &out_file, &validator_options, &optimizer_options); + optimizer_options.set_validator_options(validator_options); if (status.action == OPT_STOP) { return status.code; diff --git a/tools/val/val.cpp b/tools/val/val.cpp index 172dd121d..2f4f626ea 100644 --- a/tools/val/val.cpp +++ b/tools/val/val.cpp @@ -45,6 +45,7 @@ Options: --max-function-args --max-control-flow-nesting-depth --max-access-chain-indexes + --max-id-bound --relax-logical-pointer Allow allocating an object of a pointer type and returning a pointer value from a function in logical addressing mode --relax-block-layout Enable VK_HR_relaxed_block_layout when checking standard