From: Marcin Siodelski Date: Fri, 14 Dec 2012 16:42:50 +0000 (+0100) Subject: [2526] Changes as a result of the code review. X-Git-Tag: bind10-1.0.0-beta-release~19^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0ebb7cd975d578f3dea516b790e502fdd65cb683;p=thirdparty%2Fkea.git [2526] Changes as a result of the code review. --- diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index a76bdab39d..b19ed218d1 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -102,31 +102,35 @@ LibDHCP::optionFactory(Option::Universe u, size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, isc::dhcp::Option::OptionCollection& options) { size_t offset = 0; - size_t end = buf.size(); - - while (offset +4 <= end) { - uint16_t opt_type = buf[offset] * 256 + buf[offset + 1]; + size_t length = buf.size(); + + // Get the list of stdandard option definitions. + const OptionDefContainer& option_defs = LibDHCP::getOptionDefs(Option::V6); + // Get the search index #1. It allows to search for option definitions + // using option code. + const OptionDefContainerTypeIndex& idx = option_defs.get<1>(); + + // The buffer being read comprises a set of options, each starting with + // a two-byte type code and a two-byte length field. + while (offset + 4 <= length) { + uint16_t opt_type = isc::util::readUint16(&buf[offset]); offset += 2; - uint16_t opt_len = buf[offset] * 256 + buf[offset + 1]; + uint16_t opt_len = isc::util::readUint16(&buf[offset]); offset += 2; - if (offset + opt_len > end) { + if (offset + opt_len > length) { // @todo: consider throwing exception here. return (offset); } - // Get the list of stdandard option definitions. - OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V6); - // Get the search index #1. It allows to search for option definitions - // using option code. - const OptionDefContainerTypeIndex& idx = option_defs.get<1>(); - // Get all options with the particular option code. Note that option code - // is non-unique within this container however at this point we expect - // to get one option definition with the particular code. If more are - // returned we report an error. + // Get all definitions with the particular option code. Note that option + // code is non-unique within this container however at this point we + // expect to get one option definition with the particular code. If more + // are returned we report an error. const OptionDefContainerTypeRange& range = idx.equal_range(opt_type); // Get the number of returned option definitions for the option code. size_t num_defs = distance(range.first, range.second); + OptionPtr opt; if (num_defs > 1) { // Multiple options of the same code are not supported right now! @@ -164,7 +168,14 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, isc::dhcp::Option::OptionCollection& options) { size_t offset = 0; - // 2 byte - header of DHCPv4 option + // Get the list of stdandard option definitions. + const OptionDefContainer& option_defs = LibDHCP::getOptionDefs(Option::V4); + // Get the search index #1. It allows to search for option definitions + // using option code. + const OptionDefContainerTypeIndex& idx = option_defs.get<1>(); + + // The buffer being read comprises a set of options, each starting with + // a one-byte type code and a one-byte length field. while (offset + 1 <= buf.size()) { uint8_t opt_type = buf[offset++]; @@ -191,12 +202,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, << "-byte long buffer."); } - // Get the list of stdandard option definitions. - OptionDefContainer option_defs = LibDHCP::getOptionDefs(Option::V4); - // Get the search index #1. It allows to search for option definitions - // using option code. - const OptionDefContainerTypeIndex& idx = option_defs.get<1>(); - // Get all options with the particular option code. Note that option code + // Get all definitions with the particular option code. Note that option code // is non-unique within this container however at this point we expect // to get one option definition with the particular code. If more are // returned we report an error. @@ -204,7 +210,6 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, // Get the number of returned option definitions for the option code. size_t num_defs = distance(range.first, range.second); - OptionPtr opt; if (num_defs > 1) { // Multiple options of the same code are not supported right now! diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index 370106b7ec..c325aa5a0c 100644 --- a/src/lib/dhcp/libdhcp++.h +++ b/src/lib/dhcp/libdhcp++.h @@ -140,7 +140,7 @@ private: /// /// @throw std::bad alloc if system went out of memory. /// @throw MalformedOptionDefinition if any of the definitions - /// is incorrect. This is programming error. + /// are incorrect. This is programming error. static void initStdOptionDefs4(); /// Initialize standard DHCPv6 option definitions. diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index d8ad450ebe..818de066e2 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -150,7 +150,8 @@ private: } }; -static const uint8_t packed[] = { +// The DHCPv6 options in the wire format, used by multiple tests. +const uint8_t v6packed[] = { 0, 1, 0, 5, 100, 101, 102, 103, 104, // CLIENT_ID (9 bytes) 0, 2, 0, 3, 105, 106, 107, // SERVER_ID (7 bytes) 0, 14, 0, 0, // RAPID_COMMIT (0 bytes) @@ -254,8 +255,8 @@ TEST_F(LibDhcpTest, packOptions6) { OutputBuffer assembled(512); EXPECT_NO_THROW(LibDHCP::packOptions6(assembled, opts)); - EXPECT_EQ(sizeof(packed), assembled.getLength()); - EXPECT_EQ(0, memcmp(assembled.getData(), packed, sizeof(packed))); + EXPECT_EQ(sizeof(v6packed), assembled.getLength()); + EXPECT_EQ(0, memcmp(assembled.getData(), v6packed, sizeof(v6packed))); } TEST_F(LibDhcpTest, unpackOptions6) { @@ -267,10 +268,10 @@ TEST_F(LibDhcpTest, unpackOptions6) { isc::dhcp::Option::OptionCollection options; // list of options OptionBuffer buf(512); - memcpy(&buf[0], packed, sizeof(packed)); + memcpy(&buf[0], v6packed, sizeof(v6packed)); EXPECT_NO_THROW ({ - LibDHCP::unpackOptions6(OptionBuffer(buf.begin(), buf.begin() + sizeof(packed)), + LibDHCP::unpackOptions6(OptionBuffer(buf.begin(), buf.begin() + sizeof(v6packed)), options); }); @@ -281,14 +282,14 @@ TEST_F(LibDhcpTest, unpackOptions6) { EXPECT_EQ(1, x->second->getType()); // this should be option 1 ASSERT_EQ(9, x->second->len()); // it should be of length 9 ASSERT_EQ(5, x->second->getData().size()); - EXPECT_EQ(0, memcmp(&x->second->getData()[0], packed + 4, 5)); // data len=5 + EXPECT_EQ(0, memcmp(&x->second->getData()[0], v6packed + 4, 5)); // data len=5 x = options.find(2); ASSERT_FALSE(x == options.end()); // option 2 should exist EXPECT_EQ(2, x->second->getType()); // this should be option 2 ASSERT_EQ(7, x->second->len()); // it should be of length 7 ASSERT_EQ(3, x->second->getData().size()); - EXPECT_EQ(0, memcmp(&x->second->getData()[0], packed + 13, 3)); // data len=3 + EXPECT_EQ(0, memcmp(&x->second->getData()[0], v6packed + 13, 3)); // data len=3 x = options.find(14); ASSERT_FALSE(x == options.end()); // option 14 should exist @@ -316,7 +317,7 @@ TEST_F(LibDhcpTest, unpackOptions6) { expected_opts.push_back(0x6C6D); // equivalent to: 108, 109 expected_opts.push_back(0x6E6F); // equivalent to 110, 111 ASSERT_EQ(expected_opts.size(), opts.size()); - // Validated if option has been unpacked correctly. + // Validated if option has been un packed correctly. EXPECT_TRUE(std::equal(expected_opts.begin(), expected_opts.end(), opts.begin())); @@ -395,11 +396,11 @@ TEST_F(LibDhcpTest, packOptions4) { TEST_F(LibDhcpTest, unpackOptions4) { - vector packed(v4Opts, v4Opts + sizeof(v4Opts)); + vector v4packed(v4Opts, v4Opts + sizeof(v4Opts)); isc::dhcp::Option::OptionCollection options; // list of options ASSERT_NO_THROW( - LibDHCP::unpackOptions4(packed, options); + LibDHCP::unpackOptions4(v4packed, options); ); isc::dhcp::Option::OptionCollection::const_iterator x = options.find(12); diff --git a/src/lib/dhcp/tests/option_int_unittest.cc b/src/lib/dhcp/tests/option_int_unittest.cc index 73659d9098..81ebcf0fe4 100644 --- a/src/lib/dhcp/tests/option_int_unittest.cc +++ b/src/lib/dhcp/tests/option_int_unittest.cc @@ -457,7 +457,10 @@ TEST_F(OptionIntTest, unpackSuboptions4) { 0x01, 0x02, 0x03, 0x04, // data, uint32_t value = 0x01020304 TEST_OPT_CODE + 1, 0x4, 0x01, 0x01, 0x01, 0x01 // suboption }; - // Copy the data to a vector so as we can pas it to the + // Make sure that the buffer size is sufficient to copy the + // elements from the array. + ASSERT_GE(buf_.size(), sizeof(expected)); + // Copy the data to a vector so as we can pass it to the // OptionInt's constructor. memcpy(&buf_[0], expected, sizeof(expected)); @@ -513,6 +516,9 @@ TEST_F(OptionIntTest, unpackSuboptions6) { }; ASSERT_EQ(38, sizeof(expected)); + // Make sure that the buffer's size is sufficient to + // copy the elements from the array. + ASSERT_GE(buf_.size(), sizeof(expected)); memcpy(&buf_[0], expected, sizeof(expected)); boost::shared_ptr > opt; diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index eccc0397d0..e484dcce35 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -544,11 +544,6 @@ TEST(Pkt4Test, unpackOptions) { boost::shared_ptr pkt(new Pkt4(&expectedFormat[0], expectedFormat.size())); - try { - pkt->unpack(); - } catch (const Exception& ex) { - std::cout << ex.what() << std::endl; - } EXPECT_NO_THROW( pkt->unpack() );