[2490] Improvements to option definition validation function.

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());
     }
 }
 

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;
 

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
     );
 }