[master] Merge branch 'trac2723' (too short/null DUIDs handing)

Conflicts:
	ChangeLog
	src/lib/dhcpsrv/mysql_lease_mgr.cc

ChangeLog

@@ -1,3 +1,8 @@
+595.	[bug]		tomek
+	All DHCP components now gracefully refuse to handle too short
+	DUIDs and client-id.
+	(Trac #2723, git a043d8ecda6aff57922fe98a33c7c3f6155d5d64)
+
 594.	[func]		muks, pselkirk
 	libdns++: the NSEC, DS, DLV, and AFSDB Rdata classes now use the
 	generic lexer in constructors from text.  This means that the name

src/lib/dhcp/duid.cc

@@ -28,15 +28,20 @@ namespace dhcp {
 DUID::DUID(const std::vector<uint8_t>& duid) {
     if (duid.size() > MAX_DUID_LEN) {
         isc_throw(OutOfRange, "DUID too large");
-    } else {
-        duid_ = duid;
     }
+    if (duid.empty()) {
+        isc_throw(OutOfRange, "Empty DUIDs are not allowed");
+    }
+    duid_ = duid;
 }
 
 DUID::DUID(const uint8_t* data, size_t len) {
     if (len > MAX_DUID_LEN) {
         isc_throw(OutOfRange, "DUID too large");
     }
+    if (len == 0) {
+        isc_throw(OutOfRange, "Empty DUIDs/Client-ids not allowed");
+    }
 
     duid_ = std::vector<uint8_t>(data, data + len);
 }
@@ -83,11 +88,19 @@ bool DUID::operator!=(const DUID& other) const {
 // Constructor based on vector<uint8_t>
 ClientId::ClientId(const std::vector<uint8_t>& clientid)
     : DUID(clientid) {
+    if (clientid.size() < MIN_CLIENT_ID_LEN) {
+        isc_throw(OutOfRange, "client-id is too short (" << clientid.size()
+                  << "), at least 2 is required");
+    }
 }
 
 // Constructor based on C-style data
 ClientId::ClientId(const uint8_t *clientid, size_t len)
     : DUID(clientid, len) {
+    if (len < MIN_CLIENT_ID_LEN) {
+        isc_throw(OutOfRange, "client-id is too short (" << len
+                  << "), at least 2 is required");
+    }
 }
 
 // Returns a copy of client-id data

src/lib/dhcp/duid.h

@@ -35,6 +35,11 @@ class DUID {
     /// As defined in RFC3315, section 9.1
     static const size_t MAX_DUID_LEN = 128;
 
+    /// @brief minimum duid size
+    /// There's no explicit minimal DUID size specified in RFC3315,
+    /// so let's use absolute minimum
+    static const size_t MIN_DUID_LEN = 1;
+
     /// @brief specifies DUID type
     typedef enum {
         DUID_UNKNOWN = 0, ///< invalid/unknown type
@@ -88,6 +93,13 @@ typedef boost::shared_ptr<DUID> DuidPtr;
 /// a client-id
 class ClientId : public DUID {
 public:
+
+    /// @brief Minimum size of a client ID
+    ///
+    /// Excerpt from RFC2132, section 9.14.
+    /// The code for this option is 61, and its minimum length is 2.
+    static const size_t MIN_CLIENT_ID_LEN = 2;
+
     /// @brief Maximum size of a client ID
     ///
     /// This is the same as the maximum size of the underlying DUID.

src/lib/dhcp/tests/duid_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2013  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -37,6 +37,15 @@ using boost::scoped_ptr;
 
 namespace {
 
+// This is a workaround for strange linking problems with gtest:
+// libdhcp___unittests-duid_unittest.o: In function `Compare<long unsigned int, long unsigned int>':
+// ~/gtest-1.6.0/include/gtest/gtest.h:1353: undefined reference to `isc::dhcp::ClientId::MAX_CLIENT_ID_LE'N
+// collect2: ld returned 1 exit status
+
+const size_t MAX_DUID_LEN = DUID::MAX_DUID_LEN;
+const size_t MAX_CLIENT_ID_LEN = DUID::MAX_DUID_LEN;
+
+
 // This test verifies if the constructors are working as expected
 // and process passed parameters.
 TEST(DuidTest, constructor) {
@@ -61,21 +70,20 @@ TEST(DuidTest, constructor) {
 // 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];
+    uint8_t data[MAX_DUID_LEN + 1];
     vector<uint8_t> data2;
-    for (uint8_t i = 0; i < MAX_DUID_SIZE + 1; ++i) {
+    for (uint8_t i = 0; i < MAX_DUID_LEN + 1; ++i) {
         data[i] = i;
-        if (i < MAX_DUID_SIZE)
+        if (i < MAX_DUID_LEN)
             data2.push_back(i);
     }
-    ASSERT_EQ(data2.size(), MAX_DUID_SIZE);
+    ASSERT_EQ(data2.size(), MAX_DUID_LEN);
 
-    scoped_ptr<DUID> duidmaxsize1(new DUID(data, MAX_DUID_SIZE));
+    scoped_ptr<DUID> duidmaxsize1(new DUID(data, MAX_DUID_LEN));
     scoped_ptr<DUID> duidmaxsize2(new DUID(data2));
 
     EXPECT_THROW(
-        scoped_ptr<DUID> toolarge1(new DUID(data, MAX_DUID_SIZE + 1)),
+        scoped_ptr<DUID> toolarge1(new DUID(data, MAX_DUID_LEN + 1)),
         OutOfRange);
 
     // that's one too much
@@ -84,6 +92,16 @@ TEST(DuidTest, size) {
     EXPECT_THROW(
         scoped_ptr<DUID> toolarge2(new DUID(data2)),
         OutOfRange);
+
+    // empty duids are not allowed
+    vector<uint8_t> empty;
+    EXPECT_THROW(
+        scoped_ptr<DUID> emptyDuid(new DUID(empty)),
+        OutOfRange);
+
+    EXPECT_THROW(
+        scoped_ptr<DUID> emptyDuid2(new DUID(data, 0)),
+        OutOfRange);
 }
 
 // This test verifies if the implementation supports all defined
@@ -157,6 +175,51 @@ TEST(ClientIdTest, constructor) {
     EXPECT_TRUE(data2 == vecdata);
 }
 
+// Check that client-id sizes are reasonable
+TEST(ClientIdTest, size) {
+    uint8_t data[MAX_CLIENT_ID_LEN + 1];
+    vector<uint8_t> data2;
+    for (uint8_t i = 0; i < MAX_CLIENT_ID_LEN + 1; ++i) {
+        data[i] = i;
+        if (i < MAX_CLIENT_ID_LEN)
+            data2.push_back(i);
+    }
+    ASSERT_EQ(data2.size(), MAX_CLIENT_ID_LEN);
+
+    scoped_ptr<ClientId> duidmaxsize1(new ClientId(data, MAX_CLIENT_ID_LEN));
+    scoped_ptr<ClientId> duidmaxsize2(new ClientId(data2));
+
+    EXPECT_THROW(
+        scoped_ptr<ClientId> toolarge1(new ClientId(data, MAX_CLIENT_ID_LEN + 1)),
+        OutOfRange);
+
+    // that's one too much
+    data2.push_back(128);
+
+    EXPECT_THROW(
+        scoped_ptr<ClientId> toolarge2(new ClientId(data2)),
+        OutOfRange);
+
+    // empty client-ids are not allowed
+    vector<uint8_t> empty;
+    EXPECT_THROW(
+        scoped_ptr<ClientId> empty_client_id1(new ClientId(empty)),
+        OutOfRange);
+
+    EXPECT_THROW(
+        scoped_ptr<ClientId> empty_client_id2(new ClientId(data, 0)),
+        OutOfRange);
+
+    // client-id must be at least 2 bytes long
+    vector<uint8_t> shorty(1,17); // just a single byte with value 17
+    EXPECT_THROW(
+        scoped_ptr<ClientId> too_short_client_id1(new ClientId(shorty)),
+        OutOfRange);
+    EXPECT_THROW(
+        scoped_ptr<ClientId> too_short_client_id1(new ClientId(data, 1)),
+        OutOfRange);
+}
+
 // This test checks if the comparison operators are sane.
 TEST(ClientIdTest, operators) {
     uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};

src/lib/dhcpsrv/alloc_engine.cc

@@ -177,6 +177,14 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet,
             isc_throw(InvalidOperation, "No allocator selected");
         }
 
+        if (!subnet) {
+            isc_throw(InvalidOperation, "Subnet is required for allocation");
+        }
+
+        if (!duid) {
+            isc_throw(InvalidOperation, "DUID is mandatory for allocation");
+        }
+
         // check if there's existing lease for that subnet/duid/iaid combination.
         Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(*duid, iaid, subnet->getID());
         if (existing) {
@@ -284,6 +292,14 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet,
             isc_throw(InvalidOperation, "No allocator selected");
         }
 
+        if (!subnet) {
+            isc_throw(InvalidOperation, "Can't allocate IPv4 address without subnet");
+        }
+
+        if (!hwaddr) {
+            isc_throw(InvalidOperation, "HWAddr must be defined");
+        }
+
         // Check if there's existing lease for that subnet/clientid/hwaddr combination.
         Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet->getID());
         if (existing) {

src/lib/dhcpsrv/lease_mgr.h

@@ -249,8 +249,10 @@ struct Lease4 : public Lease {
            const uint8_t* clientid, size_t clientid_len, uint32_t valid_lft,
            uint32_t t1, uint32_t t2, time_t cltt, uint32_t subnet_id)
         : Lease(addr, t1, t2, valid_lft, subnet_id, cltt),
-        ext_(0), hwaddr_(hwaddr, hwaddr + hwaddr_len),
-        client_id_(new ClientId(clientid, clientid_len)) {
+        ext_(0), hwaddr_(hwaddr, hwaddr + hwaddr_len) {
+        if (clientid_len) {
+            client_id_.reset(new ClientId(clientid, clientid_len));
+        }
     }
 
     /// @brief Default constructor

src/lib/dhcpsrv/mysql_lease_mgr.cc

@@ -315,6 +315,10 @@ public:
         lease_ = lease;
 
         // Initialize prior to constructing the array of MYSQL_BIND structures.
+        // It sets all fields, including is_null, to zero, so we need to set
+        // is_null only if it should be true. This gives up minor performance
+        // benefit while being safe approach. For improved readability, the
+        // code that explicitly sets is_null is there, but is commented out.
         memset(bind_, 0, sizeof(bind_));
 
         // Set up the structures for the various components of the lease4
@@ -327,6 +331,8 @@ public:
         bind_[0].buffer_type = MYSQL_TYPE_LONG;
         bind_[0].buffer = reinterpret_cast<char*>(&addr4_);
         bind_[0].is_unsigned = MLM_TRUE;
+        // bind_[0].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // hwaddr: varbinary(128)
         // For speed, we avoid copying the data into temporary storage and
@@ -336,6 +342,8 @@ public:
         bind_[1].buffer = reinterpret_cast<char*>(&(lease_->hwaddr_[0]));
         bind_[1].buffer_length = hwaddr_length_;
         bind_[1].length = &hwaddr_length_;
+        // bind_[1].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // client_id: varbinary(128)
         if (lease_->client_id_) {
@@ -345,6 +353,8 @@ public:
             bind_[2].buffer = reinterpret_cast<char*>(&client_id_[0]);
             bind_[2].buffer_length = client_id_length_;
             bind_[2].length = &client_id_length_;
+            // bind_[2].is_null = &MLM_FALSE; // commented out for performance
+                                              // reasons, see memset() above
         } else {
             bind_[2].buffer_type = MYSQL_TYPE_NULL;
 
@@ -353,15 +363,17 @@ public:
             // fields doesn't matter if type is set to MYSQL_TYPE_NULL,
             // but let's set them to some sane values in case earlier versions
             // didn't have that assumption.
-            static my_bool no_clientid = MLM_TRUE;
+            client_id_null_ = MLM_TRUE;
             bind_[2].buffer = NULL;
-            bind_[2].is_null = &no_clientid;
+            bind_[2].is_null = &client_id_null_;
         }
 
         // valid lifetime: unsigned int
         bind_[3].buffer_type = MYSQL_TYPE_LONG;
         bind_[3].buffer = reinterpret_cast<char*>(&lease_->valid_lft_);
         bind_[3].is_unsigned = MLM_TRUE;
+        // bind_[3].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // expire: timestamp
         // The lease structure holds the client last transmission time (cltt_)
@@ -377,12 +389,16 @@ public:
         bind_[4].buffer_type = MYSQL_TYPE_TIMESTAMP;
         bind_[4].buffer = reinterpret_cast<char*>(&expire_);
         bind_[4].buffer_length = sizeof(expire_);
+        // bind_[4].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // subnet_id: unsigned int
         // Can use lease_->subnet_id_ directly as it is of type uint32_t.
         bind_[5].buffer_type = MYSQL_TYPE_LONG;
         bind_[5].buffer = reinterpret_cast<char*>(&lease_->subnet_id_);
         bind_[5].is_unsigned = MLM_TRUE;
+        // bind_[5].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // Add the error flags
         setErrorIndicators(bind_, error_, LEASE_COLUMNS);
@@ -404,12 +420,18 @@ public:
     std::vector<MYSQL_BIND> createBindForReceive() {
 
         // Initialize MYSQL_BIND array.
+        // It sets all fields, including is_null, to zero, so we need to set
+        // is_null only if it should be true. This gives up minor performance
+        // benefit while being safe approach. For improved readability, the
+        // code that explicitly sets is_null is there, but is commented out.
         memset(bind_, 0, sizeof(bind_));
 
         // address:  uint32_t
         bind_[0].buffer_type = MYSQL_TYPE_LONG;
         bind_[0].buffer = reinterpret_cast<char*>(&addr4_);
         bind_[0].is_unsigned = MLM_TRUE;
+        // bind_[0].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // hwaddr: varbinary(20)
         hwaddr_length_ = sizeof(hwaddr_buffer_);
@@ -417,6 +439,8 @@ public:
         bind_[1].buffer = reinterpret_cast<char*>(hwaddr_buffer_);
         bind_[1].buffer_length = hwaddr_length_;
         bind_[1].length = &hwaddr_length_;
+        // bind_[1].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // client_id: varbinary(128)
         client_id_length_ = sizeof(client_id_buffer_);
@@ -424,21 +448,30 @@ public:
         bind_[2].buffer = reinterpret_cast<char*>(client_id_buffer_);
         bind_[2].buffer_length = client_id_length_;
         bind_[2].length = &client_id_length_;
+        bind_[2].is_null = &client_id_null_;
+        // bind_[2].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // lease_time: unsigned int
         bind_[3].buffer_type = MYSQL_TYPE_LONG;
         bind_[3].buffer = reinterpret_cast<char*>(&valid_lifetime_);
         bind_[3].is_unsigned = MLM_TRUE;
+        // bind_[3].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // expire: timestamp
         bind_[4].buffer_type = MYSQL_TYPE_TIMESTAMP;
         bind_[4].buffer = reinterpret_cast<char*>(&expire_);
         bind_[4].buffer_length = sizeof(expire_);
+        // bind_[4].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // subnet_id: unsigned int
         bind_[5].buffer_type = MYSQL_TYPE_LONG;
         bind_[5].buffer = reinterpret_cast<char*>(&subnet_id_);
         bind_[5].is_unsigned = MLM_TRUE;
+        // bind_[5].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // Add the error flags
         setErrorIndicators(bind_, error_, LEASE_COLUMNS);
@@ -465,6 +498,11 @@ public:
         time_t cltt = 0;
         MySqlLeaseMgr::convertFromDatabaseTime(expire_, valid_lifetime_, cltt);
 
+        if (client_id_null_==MLM_TRUE) {
+            // There's no client-id, so we pass client-id_length_ set to 0
+            client_id_length_ = 0;
+        }
+
         // note that T1 and T2 are not stored
         return (Lease4Ptr(new Lease4(addr4_, hwaddr_buffer_, hwaddr_length_,
                                      client_id_buffer_, client_id_length_,
@@ -502,6 +540,8 @@ private:
     uint8_t         client_id_buffer_[ClientId::MAX_CLIENT_ID_LEN];
                                         ///< Client ID buffer
     unsigned long   client_id_length_;  ///< Client ID address length
+    my_bool         client_id_null_;    ///< Is Client ID null?
+
     MYSQL_TIME      expire_;            ///< Lease expiry time
     Lease4Ptr       lease_;             ///< Pointer to lease object
     uint32_t        subnet_id_;         ///< Subnet identification
@@ -564,6 +604,10 @@ public:
 
         // Ensure bind_ array clear for constructing the MYSQL_BIND structures
         // for this lease.
+        // It sets all fields, including is_null, to zero, so we need to set
+        // is_null only if it should be true. This gives up minor performance
+        // benefit while being safe approach. For improved readability, the
+        // code that explicitly sets is_null is there, but is commented out.
         memset(bind_, 0, sizeof(bind_));
 
         // address: varchar(39)
@@ -588,6 +632,8 @@ public:
         bind_[0].buffer = const_cast<char*>(addr6_.c_str());
         bind_[0].buffer_length = addr6_length_;
         bind_[0].length = &addr6_length_;
+        // bind_[0].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // duid: varchar(128)
         duid_ = lease_->duid_->getDuid();
@@ -597,11 +643,15 @@ public:
         bind_[1].buffer = reinterpret_cast<char*>(&(duid_[0]));
         bind_[1].buffer_length = duid_length_;
         bind_[1].length = &duid_length_;
+        // bind_[1].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // valid lifetime: unsigned int
         bind_[2].buffer_type = MYSQL_TYPE_LONG;
         bind_[2].buffer = reinterpret_cast<char*>(&lease_->valid_lft_);
         bind_[2].is_unsigned = MLM_TRUE;
+        // bind_[2].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // expire: timestamp
         // The lease structure holds the client last transmission time (cltt_)
@@ -616,18 +666,24 @@ public:
         bind_[3].buffer_type = MYSQL_TYPE_TIMESTAMP;
         bind_[3].buffer = reinterpret_cast<char*>(&expire_);
         bind_[3].buffer_length = sizeof(expire_);
+        // bind_[3].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // subnet_id: unsigned int
         // Can use lease_->subnet_id_ directly as it is of type uint32_t.
         bind_[4].buffer_type = MYSQL_TYPE_LONG;
         bind_[4].buffer = reinterpret_cast<char*>(&lease_->subnet_id_);
         bind_[4].is_unsigned = MLM_TRUE;
+        // bind_[4].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // pref_lifetime: unsigned int
         // Can use lease_->preferred_lft_ directly as it is of type uint32_t.
         bind_[5].buffer_type = MYSQL_TYPE_LONG;
         bind_[5].buffer = reinterpret_cast<char*>(&lease_->preferred_lft_);
         bind_[5].is_unsigned = MLM_TRUE;
+        // bind_[5].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // lease_type: tinyint
         // Must convert to uint8_t as lease_->type_ is a LeaseType variable.
@@ -635,18 +691,24 @@ public:
         bind_[6].buffer_type = MYSQL_TYPE_TINY;
         bind_[6].buffer = reinterpret_cast<char*>(&lease_type_);
         bind_[6].is_unsigned = MLM_TRUE;
+        // bind_[6].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // iaid: unsigned int
         // Can use lease_->iaid_ directly as it is of type uint32_t.
         bind_[7].buffer_type = MYSQL_TYPE_LONG;
         bind_[7].buffer = reinterpret_cast<char*>(&lease_->iaid_);
         bind_[7].is_unsigned = MLM_TRUE;
+        // bind_[7].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // prefix_len: unsigned tinyint
         // Can use lease_->prefixlen_ directly as it is uint32_t.
         bind_[8].buffer_type = MYSQL_TYPE_TINY;
         bind_[8].buffer = reinterpret_cast<char*>(&lease_->prefixlen_);
         bind_[8].is_unsigned = MLM_TRUE;
+        // bind_[8].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // Add the error flags
         setErrorIndicators(bind_, error_, LEASE_COLUMNS);
@@ -670,6 +732,10 @@ public:
     std::vector<MYSQL_BIND> createBindForReceive() {
 
         // Initialize MYSQL_BIND array.
+        // It sets all fields, including is_null, to zero, so we need to set
+        // is_null only if it should be true. This gives up minor performance
+        // benefit while being safe approach. For improved readability, the
+        // code that explicitly sets is_null is there, but is commented out.
         memset(bind_, 0, sizeof(bind_));
 
         // address:  varchar(39)
@@ -681,6 +747,8 @@ public:
         bind_[0].buffer = addr6_buffer_;
         bind_[0].buffer_length = addr6_length_;
         bind_[0].length = &addr6_length_;
+        // bind_[0].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // client_id: varbinary(128)
         duid_length_ = sizeof(duid_buffer_);
@@ -688,41 +756,57 @@ public:
         bind_[1].buffer = reinterpret_cast<char*>(duid_buffer_);
         bind_[1].buffer_length = duid_length_;
         bind_[1].length = &duid_length_;
+        // bind_[1].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // lease_time: unsigned int
         bind_[2].buffer_type = MYSQL_TYPE_LONG;
         bind_[2].buffer = reinterpret_cast<char*>(&valid_lifetime_);
         bind_[2].is_unsigned = MLM_TRUE;
+        // bind_[2].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // expire: timestamp
         bind_[3].buffer_type = MYSQL_TYPE_TIMESTAMP;
         bind_[3].buffer = reinterpret_cast<char*>(&expire_);
         bind_[3].buffer_length = sizeof(expire_);
+        // bind_[3].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // subnet_id: unsigned int
         bind_[4].buffer_type = MYSQL_TYPE_LONG;
         bind_[4].buffer = reinterpret_cast<char*>(&subnet_id_);
         bind_[4].is_unsigned = MLM_TRUE;
+        // bind_[4].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // pref_lifetime: unsigned int
         bind_[5].buffer_type = MYSQL_TYPE_LONG;
         bind_[5].buffer = reinterpret_cast<char*>(&pref_lifetime_);
         bind_[5].is_unsigned = MLM_TRUE;
+        // bind_[5].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // lease_type: tinyint
         bind_[6].buffer_type = MYSQL_TYPE_TINY;
         bind_[6].buffer = reinterpret_cast<char*>(&lease_type_);
         bind_[6].is_unsigned = MLM_TRUE;
+        // bind_[6].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // iaid: unsigned int
         bind_[7].buffer_type = MYSQL_TYPE_LONG;
         bind_[7].buffer = reinterpret_cast<char*>(&iaid_);
         bind_[7].is_unsigned = MLM_TRUE;
+        // bind_[7].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // prefix_len: unsigned tinyint
         bind_[8].buffer_type = MYSQL_TYPE_TINY;
         bind_[8].buffer = reinterpret_cast<char*>(&prefixlen_);
         bind_[8].is_unsigned = MLM_TRUE;
+        // bind_[8].is_null = &MLM_FALSE; // commented out for performance
+                                          // reasons, see memset() above
 
         // Add the error flags
         setErrorIndicators(bind_, error_, LEASE_COLUMNS);

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

@@ -160,7 +160,15 @@ public:
         EXPECT_EQ(subnet_->getT2(), lease->t2_);
         EXPECT_TRUE(false == lease->fqdn_fwd_);
         EXPECT_TRUE(false == lease->fqdn_rev_);
-        EXPECT_TRUE(*lease->client_id_ == *clientid_);
+        if (lease->client_id_ && !clientid_) {
+            ADD_FAILURE() << "Lease4 has a client-id, while it should have none.";
+        } else
+        if (!lease->client_id_ && clientid_) {
+            ADD_FAILURE() << "Lease4 has no client-id, but it was expected to have one.";
+        } else
+        if (lease->client_id_ && clientid_) {
+            EXPECT_TRUE(*lease->client_id_ == *clientid_);
+        }
         EXPECT_TRUE(lease->hwaddr_ == hwaddr_->hwaddr_);
         // @todo: check cltt
      }
@@ -329,6 +337,24 @@ TEST_F(AllocEngine6Test, allocBogusHint6) {
     detailCompareLease(lease, from_mgr);
 }
 
+// This test checks that NULL values are handled properly
+TEST_F(AllocEngine6Test, allocateAddress6Nulls) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    // Allocations without subnet are not allowed
+    Lease6Ptr lease = engine->allocateAddress6(Subnet6Ptr(), duid_, iaid_,
+                                               IOAddress("::"), false);
+    ASSERT_FALSE(lease);
+
+    // Allocations without DUID are not allowed either
+    lease = engine->allocateAddress6(subnet_, DuidPtr(), iaid_,
+                                     IOAddress("::"), false);
+    ASSERT_FALSE(lease);
+}
+
+
 // This test verifies that the allocator picks addresses that belong to the
 // pool
 TEST_F(AllocEngine6Test, IterativeAllocator) {
@@ -702,6 +728,42 @@ TEST_F(AllocEngine4Test, allocBogusHint4) {
 }
 
 
+// This test checks that NULL values are handled properly
+TEST_F(AllocEngine4Test, allocateAddress4Nulls) {
+    boost::scoped_ptr<AllocEngine> engine;
+    ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100)));
+    ASSERT_TRUE(engine);
+
+    // Allocations without subnet are not allowed
+    Lease4Ptr lease = engine->allocateAddress4(SubnetPtr(), clientid_, hwaddr_,
+                                               IOAddress("0.0.0.0"), false);
+    EXPECT_FALSE(lease);
+
+    // Allocations without HW address are not allowed
+    lease = engine->allocateAddress4(subnet_, clientid_, HWAddrPtr(),
+                                     IOAddress("0.0.0.0"), false);
+    EXPECT_FALSE(lease);
+
+    // Allocations without client-id are allowed
+    clientid_ = ClientIdPtr();
+    lease = engine->allocateAddress4(subnet_, ClientIdPtr(), hwaddr_,
+                                     IOAddress("0.0.0.0"), false);
+    // Check that we got a lease
+    ASSERT_TRUE(lease);
+
+    // Do all checks on the lease
+    checkLease4(lease);
+
+    // Check that the lease is indeed in LeaseMgr
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
+    ASSERT_TRUE(from_mgr);
+
+    // Now check that the lease in LeaseMgr has the same parameters
+    detailCompareLease(lease, from_mgr);
+}
+
+
+
 // This test verifies that the allocator picks addresses that belong to the
 // pool
 TEST_F(AllocEngine4Test, IterativeAllocator) {

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -307,8 +307,7 @@ public:
 
         } else if (address == straddress4_[7]) {
             lease->hwaddr_ = vector<uint8_t>();             // Empty
-            lease->client_id_ = ClientIdPtr(
-                new ClientId(vector<uint8_t>()));           // Empty
+            lease->client_id_ = ClientIdPtr();              // Empty
             lease->valid_lft_ = 7975;
             lease->cltt_ = 213876;
             lease->subnet_id_ = 19;
@@ -702,6 +701,83 @@ TEST_F(MySqlLeaseMgrTest, basicLease4) {
     detailCompareLease(leases[2], l_returned);
 }
 
+/// @brief Basic Lease4 Checks
+///
+/// Checks that the addLease, getLease4(by address), getLease4(hwaddr,subnet_id),
+/// updateLease4() and deleteLease (IPv4 address) can handle NULL client-id.
+/// (client-id is optional and may not be present)
+TEST_F(MySqlLeaseMgrTest, lease4NullClientId) {
+    // Get the leases to be used for the test.
+    vector<Lease4Ptr> leases = createLeases4();
+
+    // Let's clear client-id pointers
+    leases[1]->client_id_ = ClientIdPtr();
+    leases[2]->client_id_ = ClientIdPtr();
+    leases[3]->client_id_ = ClientIdPtr();
+
+    // Start the tests.  Add three leases to the database, read them back and
+    // check they are what we think they are.
+    EXPECT_TRUE(lmptr_->addLease(leases[1]));
+    EXPECT_TRUE(lmptr_->addLease(leases[2]));
+    EXPECT_TRUE(lmptr_->addLease(leases[3]));
+    lmptr_->commit();
+
+    // Reopen the database to ensure that they actually got stored.
+    reopen();
+
+    Lease4Ptr l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[1], l_returned);
+
+    l_returned = lmptr_->getLease4(ioaddress4_[2]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[2], l_returned);
+
+    l_returned = lmptr_->getLease4(ioaddress4_[3]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[3], l_returned);
+
+    // Check that we can't add a second lease with the same address
+    EXPECT_FALSE(lmptr_->addLease(leases[1]));
+
+    // Check that we can get the lease by HWAddr
+    HWAddr tmp(leases[2]->hwaddr_, HTYPE_ETHER);
+    Lease4Collection returned = lmptr_->getLease4(tmp);
+    ASSERT_EQ(1, returned.size());
+    detailCompareLease(leases[2], *returned.begin());
+
+    l_returned = lmptr_->getLease4(tmp, leases[2]->subnet_id_);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[2], l_returned);
+
+
+    // Check that we can update the lease
+    // Modify some fields in lease 1 (not the address) and update it.
+    ++leases[1]->subnet_id_;
+    leases[1]->valid_lft_ *= 2;
+    lmptr_->updateLease4(leases[1]);
+
+    // ... and check that the lease is indeed updated
+    l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[1], l_returned);
+
+
+
+    // Delete a lease, check that it's gone, and that we can't delete it
+    // a second time.
+    EXPECT_TRUE(lmptr_->deleteLease(ioaddress4_[1]));
+    l_returned = lmptr_->getLease4(ioaddress4_[1]);
+    EXPECT_FALSE(l_returned);
+    EXPECT_FALSE(lmptr_->deleteLease(ioaddress4_[1]));
+
+    // Check that the second address is still there.
+    l_returned = lmptr_->getLease4(ioaddress4_[2]);
+    ASSERT_TRUE(l_returned);
+    detailCompareLease(leases[2], l_returned);
+
+}
+
 /// @brief Basic Lease6 Checks
 ///
 /// Checks that the addLease, getLease6 (by address) and deleteLease (with an
@@ -781,14 +857,14 @@ TEST_F(MySqlLeaseMgrTest, getLease4Hwaddr) {
     // Repeat test with just one expected match
     // @todo: Simply use HWAddr directly once 2589 is implemented
     returned = lmptr_->getLease4(HWAddr(leases[2]->hwaddr_, HTYPE_ETHER));
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[2], *returned.begin());
 
     // Check that an empty vector is valid
     EXPECT_TRUE(leases[7]->hwaddr_.empty());
     // @todo: Simply use HWAddr directly once 2589 is implemented
     returned = lmptr_->getLease4(HWAddr(leases[7]->hwaddr_, HTYPE_ETHER));
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[7], *returned.begin());
 
     // Try to get something with invalid hardware address
@@ -958,13 +1034,14 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientId) {
 
     // Repeat test with just one expected match
     returned = lmptr_->getLease4(*leases[3]->client_id_);
-    EXPECT_EQ(1, returned.size());
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[3], *returned.begin());
 
-    // Check that an empty vector is valid
-    EXPECT_TRUE(leases[7]->client_id_->getClientId().empty());
-    returned = lmptr_->getLease4(leases[7]->hwaddr_);
-    EXPECT_EQ(1, returned.size());
+    // Check that client-id is NULL
+    EXPECT_FALSE(leases[7]->client_id_);
+    HWAddr tmp(leases[7]->hwaddr_, HTYPE_ETHER);
+    returned = lmptr_->getLease4(tmp);
+    ASSERT_EQ(1, returned.size());
     detailCompareLease(leases[7], *returned.begin());
 
     // Try to get something with invalid client ID
@@ -988,7 +1065,11 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientIdSize) {
     // ClientId::MAX_CLIENT_ID_LEN is used in an EXPECT_EQ.
     int client_id_max = ClientId::MAX_CLIENT_ID_LEN;
     EXPECT_EQ(128, client_id_max);
-    for (uint8_t i = 0; i <= client_id_max; i += 16) {
+
+    int client_id_min = ClientId::MIN_CLIENT_ID_LEN;
+    EXPECT_EQ(2, client_id_min); // See RFC2132, section 9.14
+
+    for (uint8_t i = client_id_min; i <= client_id_max; i += 16) {
         vector<uint8_t> clientid_vec(i, i);
         leases[1]->client_id_.reset(new ClientId(clientid_vec));
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
@@ -1097,13 +1178,17 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSize) {
     // For speed, go from 0 to 128 is steps of 16.
     int duid_max = DUID::MAX_DUID_LEN;
     EXPECT_EQ(128, duid_max);
-    for (uint8_t i = 0; i <= duid_max; i += 16) {
+
+    int duid_min = DUID::MIN_DUID_LEN;
+    EXPECT_EQ(1, duid_min);
+
+    for (uint8_t i = duid_min; i <= duid_max; i += 16) {
         vector<uint8_t> duid_vec(i, i);
         leases[1]->duid_.reset(new DUID(duid_vec));
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
         Lease6Collection returned = lmptr_->getLease6(*leases[1]->duid_,
                                                       leases[1]->iaid_);
-        EXPECT_EQ(1, returned.size());
+        ASSERT_EQ(1, returned.size());
         detailCompareLease(leases[1], *returned.begin());
         (void) lmptr_->deleteLease(leases[1]->addr_);
     }
@@ -1162,7 +1247,11 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSubnetIdSize) {
     // For speed, go from 0 to 128 is steps of 16.
     int duid_max = DUID::MAX_DUID_LEN;
     EXPECT_EQ(128, duid_max);
-    for (uint8_t i = 0; i <= duid_max; i += 16) {
+
+    int duid_min = DUID::MIN_DUID_LEN;
+    EXPECT_EQ(1, duid_min);
+
+    for (uint8_t i = duid_min; i <= duid_max; i += 16) {
         vector<uint8_t> duid_vec(i, i);
         leases[1]->duid_.reset(new DUID(duid_vec));
         EXPECT_TRUE(lmptr_->addLease(leases[1]));

src/lib/dhcpsrv/tests/test_utils.cc

@@ -27,7 +27,23 @@ detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second) {
     // thrown for IPv6 addresses.
     EXPECT_EQ(first->addr_.toText(), second->addr_.toText());
     EXPECT_TRUE(first->hwaddr_ == second->hwaddr_);
-    EXPECT_TRUE(*first->client_id_ == *second->client_id_);
+    if (first->client_id_ && second->client_id_) {
+        EXPECT_TRUE(*first->client_id_ == *second->client_id_);
+    } else {
+        if (first->client_id_ && !second->client_id_) {
+
+            ADD_FAILURE() << "Client-id present in first lease ("
+                          << first->client_id_->getClientId().size()
+                          << " bytes), but missing in second.";
+        }
+        if (!first->client_id_ && second->client_id_) {
+            ADD_FAILURE() << "Client-id missing in first lease, but present in second ("
+                          << second->client_id_->getClientId().size()
+                          << " bytes).";
+        }
+        // else here would mean that both leases do not have client_id_
+        // which makes them equal in that regard. It is ok.
+    }
     EXPECT_EQ(first->valid_lft_, second->valid_lft_);
     EXPECT_EQ(first->cltt_, second->cltt_);
     EXPECT_EQ(first->subnet_id_, second->subnet_id_);