Merge branch 'trac5058'

src/bin/lfc/tests/lfc_controller_unittests.cc

@@ -407,8 +407,9 @@ TEST_F(LFCControllerTest, launch4) {
     string b_3 = "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,"
                  "100,150,7,0,0,,1\n";
 
+    // This one should be invalid, no hardware address and state is not declined
     string c_1 = "192.0.2.3,,a:11:01:04,"
-                 "200,200,8,1,1,host.example.com,1\n";
+                 "200,200,8,1,1,host.example.com,0\n";
 
     string d_1 = "192.0.2.5,16:17:18:19:1a:bc,,"
                  "200,200,8,1,1,host.example.com,1\n";

src/lib/dhcp/duid.cc

@@ -62,16 +62,11 @@ DUID::fromText(const std::string& text) {
     return (DUID(binary));
 }
 
-DuidPtr
-DUID::generateEmpty() {
-
-    // Technically this is a one byte DUID with value of 0. However, we do have
-    // a number of safety checks against invalid duids (too long or empty) and
-    // we should keep them. Therefore "empty" means a single byte with value of 0.
-    std::vector<uint8_t> empty_duid(1,0);
-
-    DuidPtr duid(new DUID(empty_duid));
-    return (duid);
+const DUID&
+DUID::EMPTY() {
+    static std::vector<uint8_t> empty_duid(1,0);
+    static DUID empty(empty_duid);
+        return (empty);
 }
 
 std::string DUID::toText() const {

src/lib/dhcp/duid.h

@@ -61,17 +61,16 @@ class DUID {
     /// @return A reference to a vector holding a DUID.
     const std::vector<uint8_t>& getDuid() const;
 
-
-    /// @brief Returns an instance of empty DUID
+    /// @brief Defines the constant "empty" DUID
     ///
     /// In general, empty DUID is not allowed. The only case where it is really
     /// valid is to designate declined IPv6 Leases. We have a broad assumption
     /// that the Lease->duid_ must always be set. However, declined lease
     /// doesn't have any DUID associated with it. Hence we need a way to
     /// indicate that fact.
-    ///
-    /// @return a smart pointer to an empty DUID
-    static DuidPtr generateEmpty();
+    //
+    /// @return reference to the static constant empty DUID
+    static const DUID& EMPTY();
 
     /// @brief Returns the DUID type
     DUIDType getType() const;

src/lib/dhcp/tests/duid_unittest.cc

@@ -160,7 +160,7 @@ TEST(DuidTest, toText) {
 // This test verifies that empty DUID returns proper value
 TEST(DuidTest, empty) {
     DuidPtr empty;
-    EXPECT_NO_THROW(empty = DUID::generateEmpty());
+    EXPECT_NO_THROW(empty.reset(new DUID(DUID::EMPTY())));
 
     // This method must return something
     ASSERT_TRUE(empty);
@@ -168,6 +168,13 @@ TEST(DuidTest, empty) {
     // Ok, technically empty is not really empty, it's just a single
     // byte with value of 0.
     EXPECT_EQ("00", empty->toText());
+
+    EXPECT_TRUE(*empty == DUID::EMPTY());
+
+    uint8_t data1[] = {0, 1, 2, 3, 4, 0xff, 0xfe};
+    DUID duid(data1, sizeof(data1));
+
+    EXPECT_FALSE(duid == DUID::EMPTY());
 }
 
 // This test checks if the comparison operators are sane.

src/lib/dhcpsrv/csv_lease_file4.cc

@@ -97,6 +97,12 @@ CSVLeaseFile4::next(Lease4Ptr& lease) {
         // Get the HW address. It should never be empty and the readHWAddr checks
         // that.
         HWAddr hwaddr = readHWAddr(row);
+        uint32_t state = readState(row);
+        if (hwaddr.hwaddr_.empty() && state != Lease::STATE_DECLINED) {
+            isc_throw(isc::BadValue, "A blank hardware address is only"
+                      " valid for declined leases");
+        }
+
         lease.reset(new Lease4(readAddress(row),
                                HWAddrPtr(new HWAddr(hwaddr)),
                                client_id_vec.empty() ? NULL : &client_id_vec[0],
@@ -108,7 +114,7 @@ CSVLeaseFile4::next(Lease4Ptr& lease) {
                                readFqdnFwd(row),
                                readFqdnRev(row),
                                readHostname(row)));
-        lease->state_ = readState(row);
+        lease->state_ = state;
 
     } catch (std::exception& ex) {
         // bump the read error count
@@ -152,10 +158,6 @@ CSVLeaseFile4::readAddress(const CSVRow& row) {
 HWAddr
 CSVLeaseFile4::readHWAddr(const CSVRow& row) {
     HWAddr hwaddr = HWAddr::fromText(row.readAt(getColumnIndex("hwaddr")));
-    if (hwaddr.hwaddr_.empty()) {
-        isc_throw(isc::BadValue, "hardware address in the lease file"
-                  " must not be empty");
-    }
     return (hwaddr);
 }
 

src/lib/dhcpsrv/csv_lease_file6.cc

-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -94,7 +94,11 @@ CSVLeaseFile6::next(Lease6Ptr& lease) {
         lease->fqdn_rev_ = readFqdnRev(row);
         lease->hostname_ = readHostname(row);
         lease->state_ = readState(row);
-
+        if ((*lease->duid_ == DUID::EMPTY())
+            && lease->state_ != Lease::STATE_DECLINED) {
+            isc_throw(isc::BadValue, "The Empty DUID is"
+                      "only valid for declined leases");
+        }
     } catch (std::exception& ex) {
         // bump the read error count
         ++read_errs_;

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -459,6 +459,12 @@ replaced by those read from the file.
 % DHCPSRV_MEMFILE_LEASE_LOAD loading lease %1
 A debug message issued when DHCP lease is being loaded from the file to memory.
 
+% DHCPSRV_MEMFILE_LEASE_LOAD_ROW_ERROR discarding row %1, error: %2
+An error message issued the the DHCP lease being loaded from the given row of
+the lease file fails. The log message should contain the specific reason the
+row was discarded. The server will continue loading the remaining data.
+This may indicate a corrupt lease file.
+
 % DHCPSRV_MEMFILE_LFC_EXECUTE executing Lease File Cleanup using: %1
 An informational message issued when the Memfile lease database backend
 starts a new process to perform Lease File Cleanup.

src/lib/dhcpsrv/lease.cc

@@ -275,7 +275,7 @@ Lease6::getDuidVector() const {
 void
 Lease6::decline(uint32_t probation_period) {
     hwaddr_.reset();
-    duid_ = DUID::generateEmpty();
+    duid_.reset(new DUID(DUID::EMPTY()));
     t1_ = 0;
     t2_ = 0;
     preferred_lft_ = 0;

src/lib/dhcpsrv/lease_file_loader.h

@@ -91,6 +91,10 @@ public:
         while (true) {
             // Unable to parse the lease.
             if (!lease_file.next(lease)) {
+                LOG_ERROR(dhcpsrv_logger, DHCPSRV_MEMFILE_LEASE_LOAD_ROW_ERROR)
+                            .arg(lease_file.getReads())
+                            .arg(lease_file.getReadMsg());
+
                 // A value of 0xFFFFFFFF indicates that we don't return
                 // until the whole file is parsed, even if errors occur.
                 // Otherwise, check if we have exceeded the maximum number

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc

@@ -103,7 +103,7 @@ CSVLeaseFile4Test::writeSampleFile() const {
                   "fqdn_fwd,fqdn_rev,hostname,state\n"
                   "192.0.2.1,06:07:08:09:0a:bc,,200,200,8,1,1,"
                   "host.example.com,0\n"
-                  "192.0.2.1,,a:11:01:04,200,200,8,1,1,host.example.com,1\n"
+                  "192.0.2.1,,a:11:01:04,200,200,8,1,1,host.example.com,0\n"
                   "192.0.3.15,dd:de:ba:0d:1b:2e:3e:4f,0a:00:01:04,100,100,7,"
                   "0,0,,1\n");
 }
@@ -145,7 +145,8 @@ TEST_F(CSVLeaseFile4Test, parse) {
     EXPECT_EQ(Lease::STATE_DEFAULT, lease->state_);
     }
 
-    // Second lease is malformed - HW address is empty.
+    // Second lease is malformed - HW address is empty when state 
+    // is not declined.
     {
     SCOPED_TRACE("Second lease malformed");
     EXPECT_FALSE(lf.next(lease));
@@ -385,6 +386,34 @@ TEST_F(CSVLeaseFile4Test, downGrade) {
     }
 }
 
+// Verifies that leases with no hardware address are only permitted
+// if they are in the declined state.
+TEST_F(CSVLeaseFile4Test, declinedLeaseTest) {
+    io_.writeFile("address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
+                  "fqdn_fwd,fqdn_rev,hostname,state\n"
+                  "192.0.2.1,,,200,200,8,1,1,host.example.com,0\n"
+                  "192.0.2.1,,,200,200,8,1,1,host.example.com,1\n");
+
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
+    EXPECT_FALSE(lf.needsConversion());
+    EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState());
+    Lease4Ptr lease;
+
+    {
+    SCOPED_TRACE("No hardware and not declined, invalid");
+    EXPECT_FALSE(lf.next(lease));
+    ASSERT_FALSE(lease);
+    EXPECT_EQ(lf.getReadErrs(),1);
+    }
+
+    {
+    SCOPED_TRACE("No hardware and declined, valid");
+    EXPECT_TRUE(lf.next(lease));
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(lf.getReadErrs(),1);
+    }
+}
 
 /// @todo Currently we don't check invalid lease attributes, such as invalid
 /// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc

@@ -451,6 +451,48 @@ TEST_F(CSVLeaseFile6Test, downGrade) {
     }
 }
 
+// Verifies that leases with no DUID are invalid, and that leases
+// with the "Empty" DUID (1 byte duid = 0x0) are valid only when 
+// in the declined state.
+TEST_F(CSVLeaseFile6Test, declinedLeaseTest) {
+    io_.writeFile("address,duid,valid_lifetime,expire,subnet_id,"
+                  "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
+                  "fqdn_rev,hostname,hwaddr,state\n"
+                  "2001:db8:1::1,00,"
+                  "200,200,8,100,0,7,0,1,1,host.example.com,,0\n"
+                  "2001:db8:1::1,,"
+                  "200,200,8,100,0,7,0,1,1,host.example.com,,0\n"
+                  "2001:db8:1::1,00,"
+                  "200,200,8,100,0,7,0,1,1,host.example.com,,1\n");
+
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
+    EXPECT_FALSE(lf.needsConversion());
+    EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState());
+    Lease6Ptr lease;
+
+    {
+    SCOPED_TRACE("\"Empty\" DUID and not declined, invalid");
+    EXPECT_FALSE(lf.next(lease));
+    ASSERT_FALSE(lease);
+    EXPECT_EQ(lf.getReadErrs(),1);
+    }
+
+    {
+    SCOPED_TRACE("Missing (blank) DUID and not declined, invalid");
+    EXPECT_FALSE(lf.next(lease));
+    ASSERT_FALSE(lease);
+    EXPECT_EQ(lf.getReadErrs(),2);
+    }
+
+    {
+    SCOPED_TRACE("\"Empty\" DUID and declined, valid");
+    EXPECT_TRUE(lf.next(lease));
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(lf.getReadErrs(),2);
+    }
+}
+
 
 /// @todo Currently we don't check invalid lease attributes, such as invalid
 /// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6

src/lib/dhcpsrv/tests/lease_file_loader_unittest.cc

@@ -181,7 +181,7 @@ TEST_F(LeaseFileLoaderTest, loadWrite4) {
                       "100,135,7,0,0,,1\n";
 
     std::string c_1 = "192.0.2.3,,a:11:01:04,"
-                      "200,200,8,1,1,host.example.com,1\n";
+                      "200,200,8,1,1,host.example.com,0\n";
 
     // Create lease file with leases for 192.0.2.1, 192.0.3.15. The lease
     // entry for the 192.0.2.3 is invalid (lacks HW address) and should
@@ -427,7 +427,7 @@ TEST_F(LeaseFileLoaderTest, loadMaxErrors) {
     std::string a_2 = "192.0.2.1,06:07:08:09:0a:bc,,"
                       "200,500,8,1,1,host.example.com,1\n";
 
-    std::string b_1 = "192.0.2.3,,a:11:01:04,200,200,8,1,1,host.example.com,1\n";
+    std::string b_1 = "192.0.2.3,,a:11:01:04,200,200,8,1,1,host.example.com,0\n";
 
     std::string c_1 = "192.0.2.10,01:02:03:04:05:06,,200,300,8,1,1,,1\n";