From: Marcin Siodelski Date: Wed, 21 Nov 2012 15:58:57 +0000 (+0100) Subject: [2490] Improvements to option definition validation function. X-Git-Tag: bind10-1.0.0-beta-release~84^2^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9f48afa5e955391ea42332f2b6cc0d8f4757b438;p=thirdparty%2Fkea.git [2490] Improvements to option definition validation function. --- diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index 0af8a6e665..cd060d6634 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/src/lib/dhcp/option_definition.cc @@ -285,38 +285,42 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type, OptionBufferConstIter end) const { validate(); - if (type_ == OPT_BINARY_TYPE) { - return (factoryGeneric(u, type, begin, end)); - } else if (type_ == OPT_IPV6_ADDRESS_TYPE && array_type_) { - return (factoryAddrList6(u, type, begin, end)); - } else if (type_ == OPT_IPV4_ADDRESS_TYPE && array_type_) { - return (factoryAddrList4(u, type, begin, end)); - } else if (type_ == OPT_EMPTY_TYPE) { - return (factoryEmpty(u, type, begin, end)); - } else if (code_ == D6O_IA_NA && haveIA6Format()) { - return (factoryIA6(u, type, begin, end)); - } else if (code_ == D6O_IAADDR && haveIAAddr6Format()) { - return (factoryIAAddr6(u, type, begin, end)); - } else if (type_ == OPT_UINT8_TYPE) { - if (array_type_) { + try { + if (type_ == OPT_BINARY_TYPE) { return (factoryGeneric(u, type, begin, end)); - } else { - return (factoryInteger(u, type, begin, end)); - } - } else if (type_ == OPT_UINT16_TYPE) { - if (array_type_) { - return (factoryIntegerArray(u, type, begin, end)); - } else { - return (factoryInteger(u, type, begin, end)); - } - } else if (type_ == OPT_UINT32_TYPE) { - if (array_type_) { - return (factoryIntegerArray(u, type, begin, end)); - } else { - return (factoryInteger(u, type, begin, end)); + } else if (type_ == OPT_IPV6_ADDRESS_TYPE && array_type_) { + return (factoryAddrList6(u, type, begin, end)); + } else if (type_ == OPT_IPV4_ADDRESS_TYPE && array_type_) { + return (factoryAddrList4(u, type, begin, end)); + } else if (type_ == OPT_EMPTY_TYPE) { + return (factoryEmpty(u, type, begin, end)); + } else if (code_ == D6O_IA_NA && haveIA6Format()) { + return (factoryIA6(u, type, begin, end)); + } else if (code_ == D6O_IAADDR && haveIAAddr6Format()) { + return (factoryIAAddr6(u, type, begin, end)); + } else if (type_ == OPT_UINT8_TYPE) { + if (array_type_) { + return (factoryGeneric(u, type, begin, end)); + } else { + return (factoryInteger(u, type, begin, end)); + } + } else if (type_ == OPT_UINT16_TYPE) { + if (array_type_) { + return (factoryIntegerArray(u, type, begin, end)); + } else { + return (factoryInteger(u, type, begin, end)); + } + } else if (type_ == OPT_UINT32_TYPE) { + if (array_type_) { + return (factoryIntegerArray(u, type, begin, end)); + } else { + return (factoryInteger(u, type, begin, end)); + } } + return (factoryGeneric(u, type, begin, end)); + } catch (const Exception& ex) { + isc_throw(InvalidOptionValue, ex.what()); } - return (factoryGeneric(u, type, begin, end)); } OptionPtr @@ -364,18 +368,51 @@ OptionDefinition::sanityCheckUniverse(const Option::Universe expected_universe, void OptionDefinition::validate() const { - // Option name must not be empty. + std::ostringstream err_str; if (name_.empty()) { - isc_throw(isc::BadValue, "option name must not be empty"); - } - // Option name must not contain spaces. - if (name_.find(" ") != string::npos) { - isc_throw(isc::BadValue, "option name must not contain spaces"); + // Option name must not be empty. + err_str << "option name must not be empty"; + } else if (name_.find(" ") != string::npos) { + // Option name must not contain spaces. + err_str << "option name must not contain spaces"; + } else if (type_ >= OPT_UNKNOWN_TYPE) { + // Option definition must be of a known type. + err_str << "option type value " << type_ << " is out of range"; + } else if (type_ == OPT_STRING_TYPE && array_type_) { + // Array of strings is not allowed because there is no way + // to determine the size of a particular string and thus there + // it no way to tell when other data fields begin. + err_str << "array of strings is not a valid option definition"; + } else if (type_ == OPT_RECORD_TYPE) { + // At least two data fields should be added to the record. Otherwise + // non-record option definition could be used. + if (getRecordFields().size() < 2) { + err_str << "invalid number of data fields: " << getRecordFields().size() + << " specified for the option of type 'record'. Expected at" + << " least 2 fields."; + } else { + // If the number of fields is valid we have to check if their order + // is valid too. We check that string data fields are not laid before + // other fields. But we allow that they are laid at the end of + // an option. + const RecordFieldsCollection& fields = getRecordFields(); + for (RecordFieldsConstIter it = fields.begin(); + it != fields.end(); ++it) { + if (*it == OPT_STRING_TYPE && + it < fields.end() - 1) { + err_str << "string data field can't be laid before data fields" + << " of other types."; + break; + } + } + } + } - // Unsupported option types are not allowed. - if (type_ >= OPT_UNKNOWN_TYPE) { - isc_throw(isc::OutOfRange, "option type value " << type_ - << " is out of range"); + + // Non-empty error string means that we have hit the error. We throw + // exception and include error string. + if (!err_str.str().empty()) { + isc_throw(MalformedOptionDefinition, err_str.str()); } } diff --git a/src/lib/dhcp/option_definition.h b/src/lib/dhcp/option_definition.h index c80e1a7115..ec7fc9cfad 100644 --- a/src/lib/dhcp/option_definition.h +++ b/src/lib/dhcp/option_definition.h @@ -35,6 +35,13 @@ public: isc::Exception(file, line, what) { }; }; +/// @brief Exception to be thrown when option definition is invalid. +class MalformedOptionDefinition : public Exception { +public: + MalformedOptionDefinition(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { }; +}; + /// @brief Forward declaration to OptionDefinition. class OptionDefinition; @@ -271,9 +278,7 @@ public: /// @brief Check if the option definition is valid. /// - /// @throw isc::OutOfRange if invalid option type was specified. - /// @throw isc::BadValue if invalid option name was specified, - /// e.g. empty or containing spaces. + /// @throw MalformedOptionDefinition option definition is invalid. void validate() const; /// @brief Check if specified format is IA_NA option format. @@ -298,7 +303,8 @@ public: /// @param end end of the option buffer. /// /// @return instance of the DHCP option. - /// @todo list thrown exceptions. + /// @throw MalformedOptionDefinition if option definition is invalid. + /// @throw InvalidOptionValue if data for the option is invalid. OptionPtr optionFactory(Option::Universe u, uint16_t type, OptionBufferConstIter begin, OptionBufferConstIter end) const; @@ -314,7 +320,8 @@ public: /// @param buf option buffer. /// /// @return instance of the DHCP option. - /// @todo list thrown exceptions. + /// @throw MalformedOptionDefinition if option definition is invalid. + /// @throw InvalidOptionValue if data for the option is invalid. OptionPtr optionFactory(Option::Universe u, uint16_t type, const OptionBuffer& buf) const; @@ -337,7 +344,8 @@ public: /// @param values a vector of values to be used to set data for an option. /// /// @return instance of the DHCP option. - /// @todo list thrown exceptions. + /// @throw MalformedOptionDefinition if option definition is invalid. + /// @throw InvalidOptionValue if data for the option is invalid. OptionPtr optionFactory(Option::Universe u, uint16_t type, const std::vector& values) const; diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc index 52fdfd7350..9e94a22dd6 100644 --- a/src/lib/dhcp/tests/option_definition_unittest.cc +++ b/src/lib/dhcp/tests/option_definition_unittest.cc @@ -127,6 +127,15 @@ TEST_F(OptionDefinitionTest, addRecordField) { OptionDataType invalid_type = static_cast(OPT_UNKNOWN_TYPE + 10); EXPECT_THROW(opt_def.addRecordField(invalid_type), isc::BadValue); + + // It is bad if we use 'record' option type but don't specify + // at least two fields. + OptionDefinition opt_def2("OPTION_EMPTY_RECORD", 100, "record"); + EXPECT_THROW(opt_def2.validate(), MalformedOptionDefinition); + opt_def2.addRecordField("uint8"); + EXPECT_THROW(opt_def2.validate(), MalformedOptionDefinition); + opt_def2.addRecordField("uint32"); + EXPECT_NO_THROW(opt_def2.validate()); } // The purpose of this test is to check that validate() function @@ -134,29 +143,52 @@ TEST_F(OptionDefinitionTest, addRecordField) { TEST_F(OptionDefinitionTest, validate) { // Not supported option type string is not allowed. OptionDefinition opt_def1("OPTION_CLIENTID", D6O_CLIENTID, "non-existent-type"); - EXPECT_THROW(opt_def1.validate(), isc::OutOfRange); + EXPECT_THROW(opt_def1.validate(), MalformedOptionDefinition); // Not supported option type enum value is not allowed. OptionDefinition opt_def2("OPTION_CLIENTID", D6O_CLIENTID, OPT_UNKNOWN_TYPE); - EXPECT_THROW(opt_def2.validate(), isc::OutOfRange); + EXPECT_THROW(opt_def2.validate(), MalformedOptionDefinition); OptionDefinition opt_def3("OPTION_CLIENTID", D6O_CLIENTID, static_cast(OPT_UNKNOWN_TYPE + 2)); - EXPECT_THROW(opt_def3.validate(), isc::OutOfRange); + EXPECT_THROW(opt_def3.validate(), MalformedOptionDefinition); // Empty option name is not allowed. OptionDefinition opt_def4("", D6O_CLIENTID, "string"); - EXPECT_THROW(opt_def4.validate(), isc::BadValue); + EXPECT_THROW(opt_def4.validate(), MalformedOptionDefinition); // Option name must not contain spaces. OptionDefinition opt_def5(" OPTION_CLIENTID", D6O_CLIENTID, "string"); - EXPECT_THROW(opt_def5.validate(), isc::BadValue); + EXPECT_THROW(opt_def5.validate(), MalformedOptionDefinition); - OptionDefinition opt_def6("OPTION CLIENTID", D6O_CLIENTID, "string"); - EXPECT_THROW(opt_def6.validate(), isc::BadValue); + // Option name must not contain spaces. + OptionDefinition opt_def6("OPTION CLIENTID", D6O_CLIENTID, "string", true); + EXPECT_THROW(opt_def6.validate(), MalformedOptionDefinition); + + // Having array of strings does not make sense because there is no way + // to determine string's length. + OptionDefinition opt_def7("OPTION_CLIENTID", D6O_CLIENTID, "string", true); + EXPECT_THROW(opt_def7.validate(), MalformedOptionDefinition); + + // It does not make sense to have string field within the record before + // other fields because there is no way to determine the length of this + // string and thus there is no way to determine where the other field + // begins. + OptionDefinition opt_def8("OPTION_STATUS_CODE", D6O_STATUS_CODE, + "record"); + opt_def8.addRecordField("string"); + opt_def8.addRecordField("uint16"); + EXPECT_THROW(opt_def8.validate(), MalformedOptionDefinition); + + // ... but it is ok if the string value is the last one. + OptionDefinition opt_def9("OPTION_STATUS_CODE", D6O_STATUS_CODE, + "record"); + opt_def9.addRecordField("uint8"); + opt_def9.addRecordField("string"); } + // The purpose of this test is to verify that option definition // that comprises array of IPv6 addresses will return an instance // of option with a list of IPv6 addresses. @@ -205,7 +237,7 @@ TEST_F(OptionDefinitionTest, ipv6AddressArray) { // It should throw exception then. EXPECT_THROW( opt_def.optionFactory(Option::V6, D6O_NIS_SERVERS, buf), - isc::OutOfRange + InvalidOptionValue ); } @@ -302,7 +334,7 @@ TEST_F(OptionDefinitionTest, ipv4AddressArray) { buf.insert(buf.end(), 1, 1); // It should throw exception then. EXPECT_THROW(opt_def.optionFactory(Option::V4, DHO_NIS_SERVERS, buf), - isc::OutOfRange); + InvalidOptionValue); } // The purpose of this test is to verify that option definition @@ -507,13 +539,13 @@ TEST_F(OptionDefinitionTest, recordIA6) { // This should work for DHCPv6 only, try passing invalid universe value. EXPECT_THROW( opt_def.optionFactory(Option::V4, D6O_IA_NA, OptionBuffer(option6_ia_len)), - isc::BadValue + InvalidOptionValue ); // The length of the buffer must be at least 12 bytes. // Check too short buffer. EXPECT_THROW( opt_def.optionFactory(Option::V6, D6O_IA_NA, OptionBuffer(option6_ia_len - 1)), - isc::OutOfRange + InvalidOptionValue ); } @@ -554,13 +586,13 @@ TEST_F(OptionDefinitionTest, recordIAAddr6) { // This should work for DHCPv6 only, try passing invalid universe value. EXPECT_THROW( opt_def.optionFactory(Option::V4, D6O_IAADDR, OptionBuffer(option6_iaaddr_len)), - isc::BadValue + InvalidOptionValue ); // The length of the buffer must be at least 12 bytes. // Check too short buffer. EXPECT_THROW( opt_def.optionFactory(Option::V6, D6O_IAADDR, OptionBuffer(option6_iaaddr_len - 1)), - isc::OutOfRange + InvalidOptionValue ); } @@ -596,7 +628,7 @@ TEST_F(OptionDefinitionTest, recordIAAddr6Tokenized) { // This should work for DHCPv6 only, try passing in\valid universe value. EXPECT_THROW( opt_def.optionFactory(Option::V4, D6O_IAADDR, data_field_values), - isc::BadValue + InvalidOptionValue ); } @@ -620,7 +652,7 @@ TEST_F(OptionDefinitionTest, uint8) { // Try to provide zero-length buffer. Expect exception. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, D6O_PREFERENCE, OptionBuffer()), - isc::OutOfRange + InvalidOptionValue ); // @todo Add more cases for DHCPv4 @@ -676,7 +708,7 @@ TEST_F(OptionDefinitionTest, uint16) { // Try to provide zero-length buffer. Expect exception. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, D6O_ELAPSED_TIME, OptionBuffer(1)), - isc::OutOfRange + InvalidOptionValue ); // @todo Add more cases for DHCPv4 @@ -730,7 +762,7 @@ TEST_F(OptionDefinitionTest, uint32) { // Try to provide too short buffer. Expect exception. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, D6O_CLT_TIME, OptionBuffer(2)), - isc::OutOfRange + InvalidOptionValue ); // @todo Add more cases for DHCPv4 @@ -795,12 +827,12 @@ TEST_F(OptionDefinitionTest, uint16Array) { // get exception if we provide zero-length buffer. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, opt_code, OptionBuffer()), - isc::OutOfRange + InvalidOptionValue ); // Buffer length must be multiple of data type size. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, opt_code, OptionBuffer(5)), - isc::OutOfRange + InvalidOptionValue ); } @@ -868,12 +900,12 @@ TEST_F(OptionDefinitionTest, uint32Array) { // get exception if we provide zero-length buffer. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, opt_code, OptionBuffer()), - isc::OutOfRange + InvalidOptionValue ); // Buffer length must be multiple of data type size. EXPECT_THROW( option_v6 = opt_def.optionFactory(Option::V6, opt_code, OptionBuffer(5)), - isc::OutOfRange + InvalidOptionValue ); }