From: Marcin Siodelski Date: Tue, 27 Nov 2012 14:50:36 +0000 (+0100) Subject: [2312] Code cleanup in OptionCustom::createBuffers. X-Git-Tag: bind10-1.0.0-beta-release~46^2~18^2~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e4670244659f82b8fabdcaddce43d229a79f34f8;p=thirdparty%2Fkea.git [2312] Code cleanup in OptionCustom::createBuffers. --- diff --git a/src/lib/dhcp/option_custom.cc b/src/lib/dhcp/option_custom.cc index d4ffb32b1c..b090f8f1b0 100644 --- a/src/lib/dhcp/option_custom.cc +++ b/src/lib/dhcp/option_custom.cc @@ -52,57 +52,87 @@ OptionCustom::createBuffers() { std::vector buffers; OptionBuffer::iterator data = data_.begin(); - if (definition_.getType() == OPT_RECORD_TYPE) { + + OptionDataType data_type = definition_.getType(); + if (data_type == OPT_RECORD_TYPE) { + // An option comprises a record of data fields. We need to + // get types of these data fields to allocate enough space + // for each buffer. const OptionDefinition::RecordFieldsCollection& fields = definition_.getRecordFields(); + + // Go over all data fields within a record. for (OptionDefinition::RecordFieldsConstIter field = fields.begin(); field != fields.end(); ++field) { + // For fixed-size data type such as boolean, integer, even + // IP address we can use the utility function to get the required + // buffer size. int data_size = OptionDataTypeUtil::getDataTypeLen(*field); + + // For variable size types (such as string) the function above + // will return 0 so we need to do a runtime check. Since variable + // length data fields may be laid only at the end of an option we + // consume the rest of this option. Note that validate() function + // in OptionDefinition object should have checked whether the + // data fields layout is correct (that the variable string fields + // are laid at the end). if (data_size == 0) { - if (*field == OPT_IPV4_ADDRESS_TYPE) { - data_size = asiolink::V4ADDRESS_LEN; - } else if (*field == OPT_IPV6_ADDRESS_TYPE) { - data_size = asiolink::V6ADDRESS_LEN; - } else { - data_size = std::distance(data, data_.end()); - } + data_size = std::distance(data, data_.end()); } + // If we reached the end of buffer we assume that this option is + // truncated because there is no remaining data to initialize + // an option field. if (data_size == 0) { isc_throw(OutOfRange, "option buffer truncated"); } + // Store the created buffer. buffers.push_back(OptionBuffer(data, data + data_size)); + // Proceed to the next data field. data += data_size; } } else { - OptionDataType data_type = definition_.getType(); + // If option definition type is not a 'record' than definition + // type itself indicates what type of data is being held there. int data_size = OptionDataTypeUtil::getDataTypeLen(data_type); - if (data_size == 0) { - if (data_type == OPT_IPV4_ADDRESS_TYPE) { - data_size = asiolink::V4ADDRESS_LEN; - } else if (data_type == OPT_IPV6_ADDRESS_TYPE) { - data_size = asiolink::V6ADDRESS_LEN; - } - } + // The check below will fail if the input buffer is too short + // for the data size being held by this option. + // Note that data_size returned by getDataTypeLen may be zero + // if variable length data is being held by the option but + // this will not cause this check to throw exception. if (std::distance(data, data_.end()) < data_size) { isc_throw(OutOfRange, "option buffer truncated"); } + // For an array of values we are taking different path because + // we have to handle multiple buffers. if (definition_.getArrayType()) { // We don't perform other checks for data types that can't be // used together with array indicator such as strings, empty field // etc. This is because OptionDefinition::validate function should - // have checked this already. + // have checked this already. Thus data_size must be greater than + // zero. assert(data_size > 0); + // Get equal chunks of data and store as collection of buffers. + // Truncate any remaining part which length is not divisible by + // data_size. do { buffers.push_back(OptionBuffer(data, data + data_size)); data += data_size; } while (std::distance(data, data_.end()) >= data_size); } else { + // For non-arrays the data_size can be zero because + // getDataTypeLen returns zero for variable size data types + // such as strings. Simply take whole buffer. if (data_size == 0) { data_size = std::distance(data, data_.end()); } - buffers.push_back(OptionBuffer(data, data + data_size)); + if (data_size > 0 && data_type != OPT_EMPTY_TYPE) { + buffers.push_back(OptionBuffer(data, data + data_size)); + } else if (data_type != OPT_EMPTY_TYPE) { + isc_throw(OutOfRange, "option buffer truncated"); + } } } + // If everything went ok we can replace old buffer set with new ones. std::swap(buffers_, buffers); } diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc index 69da6b9a9b..146b1097a9 100644 --- a/src/lib/dhcp/option_data_types.cc +++ b/src/lib/dhcp/option_data_types.cc @@ -31,6 +31,10 @@ OptionDataTypeUtil::getDataTypeLen(const OptionDataType data_type) { case OPT_INT32_TYPE: case OPT_UINT32_TYPE: return (4); + case OPT_IPV4_ADDRESS_TYPE: + return (asiolink::V4ADDRESS_LEN); + case OPT_IPV6_ADDRESS_TYPE: + return (asiolink::V6ADDRESS_LEN); default: ; } diff --git a/src/lib/dhcp/option_data_types.h b/src/lib/dhcp/option_data_types.h index 06e8aadca9..26e0cc47c5 100644 --- a/src/lib/dhcp/option_data_types.h +++ b/src/lib/dhcp/option_data_types.h @@ -77,7 +77,7 @@ struct OptionDataTypeTraits { template<> struct OptionDataTypeTraits { static const bool valid = true; - static const int len = sizeof(OptionBuffer); + static const int len = 0; static const bool integer_type = false; static const OptionDataType type = OPT_BINARY_TYPE; }; @@ -178,15 +178,19 @@ struct OptionDataTypeTraits { class OptionDataTypeUtil { public: - /// @brief Get data type size. + /// @brief Get data type buffer length. /// /// This functionm returs the size of a particular data type. /// Values retured by this function correspond to the data type - /// sizes defined in OptionDataTypeTraits so they rather indicate + /// sizes defined in OptionDataTypeTraits (IPV4_ADDRESS_TYPE and + /// IPV6_ADDRESS_TYPE are exceptions here) so they rather indicate /// the fixed length of the data being written into the buffer, /// not the sizeof the particular data type. Thus for data types /// such as string, binary etc. for which the buffer length can't /// be determined this function returns 0. + /// In addition, this function returns the data sizes for + /// IPV4_ADDRESS_TYPE and IPV6_ADDRESS_TYPE as their buffer + /// representations have fixed data lengths: 4 and 16 respectively. /// /// @param data_type data type which size is to be returned. /// @return data type size or zero for variable length types.