[2140] DUID no longer returns unsafe references, tests updated.

src/lib/dhcp/duid.cc

@@ -37,8 +37,8 @@ DUID::DUID(const uint8_t * data, size_t len) {
     duid_ = std::vector<uint8_t>(data, data + len);
 }
 
-const std::vector<uint8_t>& DUID::getDuid() const {
-    return duid_;
+const std::vector<uint8_t> DUID::getDuid() const {
+    return (duid_);
 }
 
 DUID::DUIDType DUID::getType() const {
@@ -71,30 +71,9 @@ ClientId::ClientId(const uint8_t *clientid, size_t len)
     :DUID(clientid, len) {
 }
 
-/// constructor based on IOAddress
-ClientId::ClientId(const isc::asiolink::IOAddress& addr)
-    :DUID(std::vector<uint8_t>(4, 0)) {
-    if (addr.getFamily() != AF_INET) {
-        isc_throw(BadValue, "Client-id supports only IPv4 addresses");
-    }
-    isc::util::writeUint32(addr, &duid_[0]);
-}
-
-/// @brief returns reference to the client-id data
+/// @brief returns a copy of client-id data
 const std::vector<uint8_t> ClientId::getClientId() const {
-    return duid_;
-}
-
-isc::asiolink::IOAddress ClientId::getAddress() const {
-    if (duid_.size() != sizeof(uint32_t)) {
-        isc_throw(BadValue, "This client-id is not an IPv4 address");
-    }
-
-    return isc::asiolink::IOAddress( isc::util::readUint32(&duid_[0]) );
-}
-
-bool ClientId::isAddress() const {
-    return (duid_.size() == sizeof(uint32_t));
+    return (duid_);
 }
 
 // compares two client-ids

src/lib/dhcp/duid.h

@@ -49,9 +49,12 @@ class DUID {
 
     /// @brief returns a const reference to the actual DUID value
     ///
-    /// Note: This reference is only valid as long as the DUID
-    /// that returned it.
-    const std::vector<uint8_t>& getDuid() const;
+    /// Note: For safety reasons, this method returns a copy of data as
+    /// otherwise the reference would be only valid as long as the object that
+    /// returned it. In any case, this method should be used only sporadically.
+    /// If there are frequent uses, we must implement some other method
+    /// (e.g. storeSelf()) that will avoid data copying.
+    const std::vector<uint8_t> getDuid() const;
 
     /// @brief returns DUID type
     DUIDType getType() const;
@@ -80,25 +83,10 @@ class ClientId : DUID {
     /// constructor based on C-style data
     ClientId(const uint8_t *clientid, size_t len);
 
-    /// constructor based on IOAddress
-    ///
-    /// @throw BadValue if specified address is not IPv4
-    ClientId(const isc::asiolink::IOAddress& addr);
-
     /// @brief returns reference to the client-id data
     ///
-    /// This reference is only valid as long as the object
-    /// that returned it.
     const std::vector<uint8_t> getClientId() const;
 
-    /// @brief return an IPv4 address represented by this client-id
-    ///
-    /// @throw BadValue if this client-id is not an IPv4 address
-    isc::asiolink::IOAddress getAddress() const;
-
-    /// @brief returns if client-id is an address
-    bool isAddress() const;
-
     // compares two client-ids
     bool operator == (const ClientId& other) const;
 

src/lib/dhcp/tests/duid_unittest.cc

@@ -33,6 +33,8 @@ using boost::scoped_ptr;
 
 namespace {
 
+// This test verifies if the constructors are working as expected
+// and process passed parameters.
 TEST(DuidTest, constructor) {
 
     uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
@@ -52,6 +54,8 @@ TEST(DuidTest, constructor) {
     EXPECT_EQ(DUID::DUID_LLT, duid2->getType());
 }
 
+// This test verifies if DUID size restrictions are implemented
+// properly.
 TEST(DuidTest, size) {
     const int MAX_DUID_SIZE = 128;
     uint8_t data[MAX_DUID_SIZE + 1];
@@ -78,6 +82,8 @@ TEST(DuidTest, size) {
         OutOfRange);
 }
 
+// This test verifies if the implementation supports all defined
+// DUID types.
 TEST(DuidTest, getType) {
     uint8_t llt[] =     {0, 1, 2, 3, 4, 5, 6};
     uint8_t en[] =      {0, 2, 2, 3, 4, 5, 6};
@@ -98,6 +104,7 @@ TEST(DuidTest, getType) {
     EXPECT_EQ(DUID::DUID_UNKNOWN, duid_invalid->getType());
 }
 
+// This test checks if the comparison operators are sane.
 TEST(DuidTest, operators) {
     uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
     uint8_t data2[] = {0, 1, 2, 3, 4};
@@ -118,38 +125,27 @@ TEST(DuidTest, operators) {
     EXPECT_TRUE(*duid1 != *duid3);
 }
 
+// This test verifies if the ClientId constructors are working properly
+// and passed parameters are used
 TEST(ClientIdTest, constructor) {
     IOAddress addr2("192.0.2.1");
     IOAddress addr3("2001:db8:1::1");
 
     uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
     vector<uint8_t> data2(data1, data1 + sizeof(data1));
-    uint8_t data3[] = {192, 0 , 2, 1 };
 
     // checks for C-style construtor (uint8_t * + len)
     scoped_ptr<ClientId> id1(new ClientId(data1, sizeof(data1)));
     vector<uint8_t> vecdata = id1->getClientId();
     EXPECT_EQ(data2, vecdata);
-    EXPECT_FALSE(id1->isAddress());
-    EXPECT_THROW(id1->getAddress(), BadValue);
 
     // checks for vector-based constructor
     scoped_ptr<ClientId> id2(new ClientId(data2));
     vecdata = id2->getClientId();
     EXPECT_EQ(data2, vecdata);
-    EXPECT_FALSE(id1->isAddress());
-    EXPECT_THROW(id1->getAddress(), BadValue);
-
-    // checks for IOAddress based constructor
-    scoped_ptr<ClientId> id3(new ClientId(addr2));
-    vecdata = id3->getClientId();
-    EXPECT_TRUE(vecdata == vector<uint8_t>(data3, data3 + 4));
-    EXPECT_EQ("192.0.2.1", id3->getAddress().toText());
-
-    // should support v4 address only, v6 is a wrong address here
-    EXPECT_THROW(new ClientId(addr3), BadValue);
 }
 
+// This test checks if the comparison operators are sane.
 TEST(ClientIdTest, operators) {
     uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
     uint8_t data2[] = {0, 1, 2, 3, 4};