From: Thomas Markwalder Date: Tue, 1 Nov 2016 15:00:29 +0000 (-0400) Subject: [5058] - Addressed review comments X-Git-Tag: trac5057_base~11^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=43ad2f493b47eaacbe7861dd00023328fa2cf359;p=thirdparty%2Fkea.git [5058] - Addressed review comments Fixed typos and added decline lease checks for DHCPv6 src/lib/dhcp/duid.h src/lib/dhcp/duid.cc src/lib/dhcp/tests/duid_unittest.cc - Replaced DUID::generateEmpty() with DUID::EMPTY() which returns a constant reference to a static DUID instance. This facilitates comparisions. - Updated TEST(DuidTest,empty) accordingly src/lib/dhcpsrv/csv_lease_file6.cc CSVLeaseFile6::next() - added test to permit the Empty DUID only if state is STATE_DECLINED. src/lib/dhcpsrv/lease.cc Lease6::decline() - modified to use DUID::EMPTY(). src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc Fixed Typos src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc TEST_F(CSVLeaseFile6Test, declinedLeaseTest) - new test to verify proper handling of declined leases --- diff --git a/src/lib/dhcp/duid.cc b/src/lib/dhcp/duid.cc index 067d4e15ce..f500ad155e 100644 --- a/src/lib/dhcp/duid.cc +++ b/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 empty_duid(1,0); - - DuidPtr duid(new DUID(empty_duid)); - return (duid); +const DUID& +DUID::EMPTY() { + static std::vector empty_duid(1,0); + static DUID empty(empty_duid); + return (empty); } std::string DUID::toText() const { diff --git a/src/lib/dhcp/duid.h b/src/lib/dhcp/duid.h index 150a79e5f1..1b4e42bd6c 100644 --- a/src/lib/dhcp/duid.h +++ b/src/lib/dhcp/duid.h @@ -61,17 +61,16 @@ class DUID { /// @return A reference to a vector holding a DUID. const std::vector& 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; diff --git a/src/lib/dhcp/tests/duid_unittest.cc b/src/lib/dhcp/tests/duid_unittest.cc index 8c8a4aa7d1..8a78db1c83 100644 --- a/src/lib/dhcp/tests/duid_unittest.cc +++ b/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. diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc index 5a041b813c..c1f8cb474a 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.cc +++ b/src/lib/dhcpsrv/csv_lease_file6.cc @@ -1,4 +1,4 @@ -// 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_; diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc index 5854f06900..7dceb9db69 100644 --- a/src/lib/dhcpsrv/lease.cc +++ b/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; diff --git a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc index 9bdd3f9297..a9ccfe189e 100644 --- a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc +++ b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc @@ -146,7 +146,7 @@ TEST_F(CSVLeaseFile4Test, parse) { } // Second lease is malformed - HW address is empty when state - // is not delcined. + // is not declined. { SCOPED_TRACE("Second lease malformed"); EXPECT_FALSE(lf.next(lease)); @@ -386,7 +386,7 @@ TEST_F(CSVLeaseFile4Test, downGrade) { } } -// Verifies that leases with no hardware address or only permitted +// 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," @@ -394,12 +394,10 @@ TEST_F(CSVLeaseFile4Test, declinedLeaseTest) { "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"); - // Lease file should open and report as needing downgrade. CSVLeaseFile4 lf(filename_); ASSERT_NO_THROW(lf.open()); EXPECT_FALSE(lf.needsConversion()); - EXPECT_EQ(util::VersionedCSVFile::CURRENT, - lf.getInputSchemaState()); + EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState()); Lease4Ptr lease; { diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc index a2003cb9d2..a2533fd972 100644 --- a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc +++ b/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