BIND 10 trac2490, updated. 9f48afa5e955391ea42332f2b6cc0d8f4757b438 [2490] Improvements to option definition validation function.
BIND 10 source code commits
bind10-changes at lists.isc.org
Wed Nov 21 15:59:05 UTC 2012
The branch, trac2490 has been updated
via 9f48afa5e955391ea42332f2b6cc0d8f4757b438 (commit)
from b4dd93598f4e795c6cac151bc26caa56975fa835 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 9f48afa5e955391ea42332f2b6cc0d8f4757b438
Author: Marcin Siodelski <marcin at isc.org>
Date: Wed Nov 21 16:58:57 2012 +0100
[2490] Improvements to option definition validation function.
-----------------------------------------------------------------------
Summary of changes:
src/lib/dhcp/option_definition.cc | 115 ++++++++++++++--------
src/lib/dhcp/option_definition.h | 20 ++--
src/lib/dhcp/tests/option_definition_unittest.cc | 74 ++++++++++----
3 files changed, 143 insertions(+), 66 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index 0af8a6e..cd060d6 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<uint8_t>(u, type, begin, end));
- }
- } else if (type_ == OPT_UINT16_TYPE) {
- if (array_type_) {
- return (factoryIntegerArray<uint16_t>(u, type, begin, end));
- } else {
- return (factoryInteger<uint16_t>(u, type, begin, end));
- }
- } else if (type_ == OPT_UINT32_TYPE) {
- if (array_type_) {
- return (factoryIntegerArray<uint32_t>(u, type, begin, end));
- } else {
- return (factoryInteger<uint32_t>(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<uint8_t>(u, type, begin, end));
+ }
+ } else if (type_ == OPT_UINT16_TYPE) {
+ if (array_type_) {
+ return (factoryIntegerArray<uint16_t>(u, type, begin, end));
+ } else {
+ return (factoryInteger<uint16_t>(u, type, begin, end));
+ }
+ } else if (type_ == OPT_UINT32_TYPE) {
+ if (array_type_) {
+ return (factoryIntegerArray<uint32_t>(u, type, begin, end));
+ } else {
+ return (factoryInteger<uint32_t>(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 c80e1a7..ec7fc9c 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<std::string>& values) const;
diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc
index 52fdfd7..9e94a22 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<OptionDataType>(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<OptionDataType>(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
);
}
More information about the bind10-changes
mailing list