From 0fda6bb19a6fa83fe512dd6889b71e3ab37709bc Mon Sep 17 00:00:00 2001 From: Slava Shklyaev Date: Thu, 18 Jun 2020 18:47:06 +0100 Subject: Add HAL-level validation for CF operands of unknown size At the NDK level, we allow IF and WHILE operations where an inner or outer input or output operand has a type that is not fully specified. However, this is not allowed At the HAL level. This CL adds HAL-level validation. See http://b/132458982#comment63 Bug: 132458982 Test: NNT_static Change-Id: I54754d6241a1f8eb99717899ffd4f0ace4750060 --- nn/common/Utils.cpp | 15 +++++++++++++++ nn/common/ValidateHal.cpp | 25 +++++++++++++++---------- nn/common/include/Utils.h | 3 +++ nn/common/include/ValidateHal.h | 6 +++++- nn/runtime/ExecutionPlan.cpp | 5 +++++ nn/runtime/Manager.cpp | 4 ++-- nn/runtime/ModelBuilder.cpp | 17 +++++++++-------- 7 files changed, 54 insertions(+), 21 deletions(-) diff --git a/nn/common/Utils.cpp b/nn/common/Utils.cpp index 188436f11..1694c9c23 100644 --- a/nn/common/Utils.cpp +++ b/nn/common/Utils.cpp @@ -762,6 +762,15 @@ static bool validateIfOperation(uint32_t inputCount, const uint32_t* inputs, uin return true; } +static bool validateControlFlowOperandUnknownSize(const SubgraphValidationHelper& helper, + const Operand& operand) { + if (!helper.allowControlFlowOperationWithOperandOfUnknownSize && + !isExtensionOperandType(operand.type)) { + NN_RET_CHECK_NE(nonExtensionOperandSizeOfData(operand.type, operand.dimensions), 0u); + } + return true; +} + static bool validateWhileOperation(uint32_t inputCount, const uint32_t* inputs, uint32_t outputCount, const uint32_t* outputs, const std::vector& operands, @@ -789,6 +798,8 @@ static bool validateWhileOperation(uint32_t inputCount, const uint32_t* inputs, const Operand& innerOperand = *helper.getSubgraphInputOperand(condModelOperand, i); const Operand& outerOperand = operands[inputs[op::kFirstInput + i]]; NN_RET_CHECK(compatible(innerOperand, outerOperand)); + NN_RET_CHECK(validateControlFlowOperandUnknownSize(helper, innerOperand)); + NN_RET_CHECK(validateControlFlowOperandUnknownSize(helper, outerOperand)); } NN_RET_CHECK( validateConditionOperand(*helper.getSubgraphOutputOperand(condModelOperand, 0))); @@ -809,16 +820,20 @@ static bool validateWhileOperation(uint32_t inputCount, const uint32_t* inputs, const Operand& innerOperand = *helper.getSubgraphInputOperand(bodyModelOperand, i); const Operand& outerOperand = operands[inputs[op::kFirstInput + i]]; NN_RET_CHECK(compatible(innerOperand, outerOperand)); + NN_RET_CHECK(validateControlFlowOperandUnknownSize(helper, innerOperand)); + NN_RET_CHECK(validateControlFlowOperandUnknownSize(helper, outerOperand)); } for (uint32_t i = 0; i < inputOutputCount; ++i) { const Operand& innerOperand = *helper.getSubgraphOutputOperand(bodyModelOperand, i); const Operand& outerOperand = operands[outputs[i]]; NN_RET_CHECK(compatible(innerOperand, outerOperand)); + NN_RET_CHECK(validateControlFlowOperandUnknownSize(helper, outerOperand)); } for (uint32_t i = 0, n = inputOutputCount + stateOnlyCount; i < n; ++i) { const Operand& inputOperand = *helper.getSubgraphInputOperand(bodyModelOperand, i); const Operand& outputOperand = *helper.getSubgraphOutputOperand(bodyModelOperand, i); NN_RET_CHECK(compatible(inputOperand, outputOperand)); + NN_RET_CHECK(validateControlFlowOperandUnknownSize(helper, outputOperand)); } return true; }; diff --git a/nn/common/ValidateHal.cpp b/nn/common/ValidateHal.cpp index 5668ff5ab..e5b9b12a8 100644 --- a/nn/common/ValidateHal.cpp +++ b/nn/common/ValidateHal.cpp @@ -448,7 +448,7 @@ static HalVersion getHalVersion(const V1_3::Operation&) { template static bool validateOperations(const hidl_vec& operations, const hidl_vec& operands, - const hidl_vec& subgraphs) { + const hidl_vec& subgraphs, ValidationMode mode) { auto isValidSubgraphReference = [&subgraphs](const Operand& modelOperand) -> bool { NN_RET_CHECK(modelOperand.type == OperandType::SUBGRAPH) << "Unexpected operand type: " << toString(modelOperand.type); @@ -490,7 +490,11 @@ static bool validateOperations(const hidl_vec& operations, .getSubgraphInputCount = getInputCount, .getSubgraphOutputCount = getOutputCount, .getSubgraphInputOperand = getInputOperand, - .getSubgraphOutputOperand = getOutputOperand}); + .getSubgraphOutputOperand = getOutputOperand, + // 1.3 HAL does not support CF operations with operands of + // unknown size. See http://b/132458982#comment63. + .allowControlFlowOperationWithOperandOfUnknownSize = + mode == ValidationMode::RUNTIME}); if (error != ANEURALNETWORKS_NO_ERROR) { LOG(ERROR) << "Invalid operation " << toString(op.type); return false; @@ -687,7 +691,7 @@ static bool checkNoReferenceCycles(const V1_3::Model& model) { } template -bool validateModel(const T_Model& model) { +bool validateModel(const T_Model& model, ValidationMode mode) { NNTRACE_FULL(NNTRACE_LAYER_UTILITY, NNTRACE_PHASE_UNSPECIFIED, "validateModel"); HalVersion version = ModelToHalVersion::version; if (model.operations.size() == 0 || model.operands.size() == 0) { @@ -699,7 +703,7 @@ bool validateModel(const T_Model& model) { const hidl_vec latestVersionOperands = convertToV1_3(model.operands); return (validateOperands(model.operands, model.operandValues, model.pools, /*subgraphs=*/{}, /*allowUnspecifiedRank=*/version >= HalVersion::V1_2) && - validateOperations(model.operations, latestVersionOperands, /*subgraphs=*/{}) && + validateOperations(model.operations, latestVersionOperands, /*subgraphs=*/{}, mode) && validateModelInputOutputs(model.inputIndexes, latestVersionOperands, OperandLifeTime::SUBGRAPH_INPUT) && validateModelInputOutputs(model.outputIndexes, latestVersionOperands, @@ -707,21 +711,22 @@ bool validateModel(const T_Model& model) { validatePools(model.pools, version) && validateGraph(model)); } -template bool validateModel(const V1_0::Model& model); -template bool validateModel(const V1_1::Model& model); -template bool validateModel(const V1_2::Model& model); +template bool validateModel(const V1_0::Model& model, ValidationMode mode); +template bool validateModel(const V1_1::Model& model, ValidationMode mode); +template bool validateModel(const V1_2::Model& model, ValidationMode mode); template <> -bool validateModel(const V1_3::Model& model) { +bool validateModel(const V1_3::Model& model, ValidationMode mode) { NNTRACE_FULL(NNTRACE_LAYER_UTILITY, NNTRACE_PHASE_UNSPECIFIED, "validateModel"); if (model.main.operations.size() == 0 || model.main.operands.size() == 0) { LOG(ERROR) << "Invalid empty model."; return false; } - auto validateSubgraph = [&model](const Subgraph& subgraph) -> bool { + auto validateSubgraph = [&model, mode](const Subgraph& subgraph) -> bool { return (validateOperands(subgraph.operands, model.operandValues, model.pools, model.referenced, /*allowUnspecifiedRank=*/true) && - validateOperations(subgraph.operations, subgraph.operands, model.referenced) && + validateOperations(subgraph.operations, subgraph.operands, model.referenced, + mode) && validateModelInputOutputs(subgraph.inputIndexes, subgraph.operands, OperandLifeTime::SUBGRAPH_INPUT) && validateModelInputOutputs(subgraph.outputIndexes, subgraph.operands, diff --git a/nn/common/include/Utils.h b/nn/common/include/Utils.h index ca11c5ebc..4bbbf8215 100644 --- a/nn/common/include/Utils.h +++ b/nn/common/include/Utils.h @@ -391,6 +391,9 @@ struct SubgraphValidationHelper { std::function getSubgraphInputOperand; // Gets the specified output operand of a subgraph referenced by a given operand. std::function getSubgraphOutputOperand; + // Whether control flow operations with inner or outer input or output + // operands of unknown size are allowed. + bool allowControlFlowOperationWithOperandOfUnknownSize; }; // Returns ANEURALNETWORKS_NO_ERROR if the corresponding operation is defined and can handle the diff --git a/nn/common/include/ValidateHal.h b/nn/common/include/ValidateHal.h index 7b097fd20..32d7662ed 100644 --- a/nn/common/include/ValidateHal.h +++ b/nn/common/include/ValidateHal.h @@ -37,6 +37,10 @@ enum class HalVersion : int32_t { enum class IOType { INPUT, OUTPUT }; using PreparedModelRole = std::tuple; +// 1.3 HAL does not support control flow operations with operands of unknown size. +// See http://b/132458982#comment63. +enum class ValidationMode { DRIVER, RUNTIME }; + // Verifies that the model is valid, i.e. it is consistent, takes // only acceptable values, the constants don't extend outside the memory // regions they are part of, etc. @@ -44,7 +48,7 @@ using PreparedModelRole = std::tuple -bool validateModel(const T_Model& model); +bool validateModel(const T_Model& model, ValidationMode mode = ValidationMode::DRIVER); // Verifies that the request for the given model is valid. // IMPORTANT: This function cannot validate that OEM operation and operands diff --git a/nn/runtime/ExecutionPlan.cpp b/nn/runtime/ExecutionPlan.cpp index 1f540805f..1797475ac 100644 --- a/nn/runtime/ExecutionPlan.cpp +++ b/nn/runtime/ExecutionPlan.cpp @@ -555,6 +555,11 @@ int ExecutionStep::finishStepModel(const ModelBuilder* mainModel, bool* hasOutpu [](auto& e) { return e.second; }); NN_RETURN_IF_ERROR(mStepModel.identifyInputsAndOutputs(inputs.size(), inputs.data(), outputs.size(), outputs.data())); + // TODO: Model::finish() should use ValidationMode::RUNTIME when sending the + // step model to CpuDevice. Right now, this is harmless because the only + // difference in validation occurs with control flow operations and inputs + // or outputs of unknown size and we never send control flow operations to + // CpuDevice. We need to address this if this behavior changes (b/151634976). NN_RETURN_IF_ERROR(mStepModel.finish()); // TODO: Move compilation elsewhere? diff --git a/nn/runtime/Manager.cpp b/nn/runtime/Manager.cpp index 634cd2aec..50d118000 100644 --- a/nn/runtime/Manager.cpp +++ b/nn/runtime/Manager.cpp @@ -657,8 +657,8 @@ std::pair> CpuDevice::prepareModel( << "Should never call prepareModel with cache information on CpuDevice"; const Model model = makeModel(); - if (!validateModel(model) || !validateExecutionPreference(preference) || - !validatePriority(priority)) { + if (!validateModel(model, ValidationMode::RUNTIME) || + !validateExecutionPreference(preference) || !validatePriority(priority)) { return {ANEURALNETWORKS_OP_FAILED, nullptr}; } if (hasDeadlinePassed(deadline)) { diff --git a/nn/runtime/ModelBuilder.cpp b/nn/runtime/ModelBuilder.cpp index 8669203de..73ec1af8c 100644 --- a/nn/runtime/ModelBuilder.cpp +++ b/nn/runtime/ModelBuilder.cpp @@ -388,13 +388,14 @@ int ModelBuilder::addOperation(ANeuralNetworksOperationType type, uint32_t input auto getOutputOperand = [this](const Operand& modelOperand, uint32_t index) -> const Operand* { return &getReferencedModel(modelOperand)->getOutputOperand(index); }; - NN_RETURN_IF_ERROR(validateOperation(type, inputCount, inputs, outputCount, outputs, mOperands, - HalVersion::LATEST, - {.isValidSubgraphReference = isValidSubgraphReference, - .getSubgraphInputCount = getInputCount, - .getSubgraphOutputCount = getOutputCount, - .getSubgraphInputOperand = getInputOperand, - .getSubgraphOutputOperand = getOutputOperand})); + NN_RETURN_IF_ERROR(validateOperation( + type, inputCount, inputs, outputCount, outputs, mOperands, HalVersion::LATEST, + {.isValidSubgraphReference = isValidSubgraphReference, + .getSubgraphInputCount = getInputCount, + .getSubgraphOutputCount = getOutputCount, + .getSubgraphInputOperand = getInputOperand, + .getSubgraphOutputOperand = getOutputOperand, + .allowControlFlowOperationWithOperandOfUnknownSize = true})); uint32_t operationIndex = operationCount(); if (operationIndex >= MAX_NUMBER_OF_OPERATIONS) { @@ -523,7 +524,7 @@ int ModelBuilder::finish() { // a CONSTANT_REFERENCE operand will not have correct .poolIndex, and // validation will not work properly. const Model modelForValidation = makeHidlModel(); - if (!validateModel(modelForValidation)) { + if (!validateModel(modelForValidation, ValidationMode::RUNTIME)) { LOG(ERROR) << "ANeuralNetworksModel_finish called on invalid model"; mInvalidModel = true; return ANEURALNETWORKS_BAD_DATA; -- cgit v1.2.3