Browse Source

[2837] Changed mysql to enforce STRICT mode
Changes:
- hwaddr.h - added MAX_HWADDR_LEN
- hwaddr.cc - modified constructor to throw InvalidParameter
if input address is too big
- hwaddr_unittest.cc - added tests for new constructor throws

- lease_mgr.h - removed HWADDR_MAX
- mysql_lease_mgr.cc - added logic to set SQL mode to STRICT
- tests/mysql_lease_mgr_unittest.cc - modified tests that were
failing on STRICT mode, to expect exceptions.

Thomas Markwalder 12 years ago
parent
commit
ab624f466e

+ 16 - 2
src/lib/dhcp/hwaddr.cc

@@ -14,6 +14,7 @@
 
 #include <dhcp/hwaddr.h>
 #include <dhcp/dhcp4.h>
+#include <exceptions/exceptions.h>
 #include <iomanip>
 #include <sstream>
 #include <vector>
@@ -26,11 +27,24 @@ HWAddr::HWAddr()
 }
 
 HWAddr::HWAddr(const uint8_t* hwaddr, size_t len, uint8_t htype)
-    :hwaddr_(hwaddr, hwaddr + len), htype_(htype) {
+    :htype_(htype) {
+
+    if (len > MAX_HWADDR_LEN)
+        isc_throw(InvalidParameter, "hwaddr length exceeds MAX_HWADDR_LEN");    
+
+    hwaddr_.resize(len);
+    memcpy(&hwaddr_[0], hwaddr, len);
 }
 
 HWAddr::HWAddr(const std::vector<uint8_t>& hwaddr, uint8_t htype)
-    :hwaddr_(hwaddr), htype_(htype) {
+    :htype_(htype) {
+
+    if (hwaddr.size() > MAX_HWADDR_LEN)
+        isc_throw(InvalidParameter, 
+            "address vector size exceeds MAX_HWADDR_LEN");    
+
+    hwaddr_ = hwaddr;
+
 }
 
 std::string HWAddr::toText() const {

+ 2 - 0
src/lib/dhcp/hwaddr.h

@@ -27,6 +27,8 @@ namespace dhcp {
 /// @brief Hardware type that represents information from DHCPv4 packet
 struct HWAddr {
 public:
+    /// @brief Maximum size of a hardware address.
+    static const size_t MAX_HWADDR_LEN = 20;
 
     /// @brief default constructor
     HWAddr();

+ 11 - 1
src/lib/dhcp/tests/hwaddr_unittest.cc

@@ -40,9 +40,12 @@ TEST(HWAddrTest, constructor) {
 
     const uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6};
     const uint8_t htype = HTYPE_ETHER;
-
     vector<uint8_t> data2(data1, data1 + sizeof(data1));
 
+    // over the limit data 
+    uint8_t big_data[HWAddr::MAX_HWADDR_LEN+1]={0}; 
+    vector<uint8_t> big_data_vector(big_data, big_data + sizeof(big_data));
+
     scoped_ptr<HWAddr> hwaddr1(new HWAddr(data1, sizeof(data1), htype));
     scoped_ptr<HWAddr> hwaddr2(new HWAddr(data2, htype));
     scoped_ptr<HWAddr> hwaddr3(new HWAddr());
@@ -55,6 +58,13 @@ TEST(HWAddrTest, constructor) {
 
     EXPECT_EQ(0, hwaddr3->hwaddr_.size());
     EXPECT_EQ(htype, hwaddr3->htype_);
+
+    // check that over the limit data length throws exception 
+    EXPECT_THROW(HWAddr(big_data, sizeof(big_data), HTYPE_ETHER), 
+        InvalidParameter);
+
+    // check that over the limit vector throws exception
+    EXPECT_THROW(HWAddr(big_data_vector, HTYPE_ETHER), InvalidParameter);
 }
 
 // This test checks if the comparison operators are sane.

+ 0 - 2
src/lib/dhcpsrv/lease_mgr.h

@@ -211,8 +211,6 @@ struct Lease {
 /// would be required. As this is a critical part of the code that will be used
 /// extensively, direct access is warranted.
 struct Lease4 : public Lease {
-    /// @brief Maximum size of a hardware address
-    static const size_t HWADDR_MAX = 20;
 
     /// @brief Address extension
     ///

+ 12 - 4
src/lib/dhcpsrv/mysql_lease_mgr.cc

@@ -94,9 +94,6 @@ namespace {
 /// colon separators.
 const size_t ADDRESS6_TEXT_MAX_LEN = 39;
 
-/// @brief Maximum size of a hardware address.
-const size_t HWADDR_MAX_LEN = 20;
-
 /// @brief MySQL True/False constants
 ///
 /// Declare typed values so as to avoid problems of data conversion.  These
@@ -495,7 +492,7 @@ private:
     std::string     columns_[LEASE_COLUMNS];///< Column names
     my_bool         error_[LEASE_COLUMNS];  ///< Error array
     std::vector<uint8_t> hwaddr_;       ///< Hardware address
-    uint8_t         hwaddr_buffer_[HWADDR_MAX_LEN];
+    uint8_t         hwaddr_buffer_[HWAddr::MAX_HWADDR_LEN];
                                         ///< Hardware address buffer
     unsigned long   hwaddr_length_;     ///< Hardware address length
     std::vector<uint8_t> client_id_;    ///< Client identification
@@ -1025,6 +1022,17 @@ MySqlLeaseMgr::openDatabase() {
                   mysql_error(mysql_));
     }
 
+    // Set SQL mode options for the connection:  SQL mode governs how what 
+    // constitutes insertable data for a given column, and how to handle
+    // invalid data.  We want to ensure we get the strictest behavior and
+    // to reject invalid data with an error.
+    const char *sql_mode = "set SESSION sql_mode ='STRICT_ALL_TABLES'";
+    result = mysql_options(mysql_, MYSQL_INIT_COMMAND, sql_mode);
+    if (result != 0) {
+        isc_throw(DbOpenError, "unable to set SQL mode options: " <<
+                  mysql_error(mysql_));
+    }
+
     // Open the database.
     //
     // The option CLIENT_FOUND_ROWS is specified so that in an UPDATE,

+ 22 - 33
src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

@@ -18,6 +18,7 @@
 #include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/mysql_lease_mgr.h>
 #include <dhcpsrv/tests/test_utils.h>
+#include <exceptions/exceptions.h>
 
 #include <gtest/gtest.h>
 
@@ -807,28 +808,23 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSize) {
     vector<Lease4Ptr> leases = createLeases4();
 
     // Now add leases with increasing hardware address size.
-    for (uint8_t i = 0; i <= Lease4::HWADDR_MAX; ++i) {
+    for (uint8_t i = 0; i <= HWAddr::MAX_HWADDR_LEN; ++i) {
         leases[1]->hwaddr_.resize(i, i);
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
         // @todo: Simply use HWAddr directly once 2589 is implemented
-        Lease4Collection returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER));
+        Lease4Collection returned = 
+            lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER));
+
         ASSERT_EQ(1, returned.size());
         detailCompareLease(leases[1], *returned.begin());
         (void) lmptr_->deleteLease(leases[1]->addr_);
     }
 
-    // Expect some problem when accessing a lease that had too long a hardware
-    // address. (The 42 is a random value put in each byte of the address.)
-    // In fact the address is stored in a truncated form, so we won't find it
-    // when we look.
-    // @todo Check if there is some way of detecting that data added
-    //       to the database is truncated.  There does not appear to
-    //       be any indication in the C API.
-    leases[1]->hwaddr_.resize(Lease4::HWADDR_MAX + 100, 42);
-    EXPECT_TRUE(lmptr_->addLease(leases[1]));
-    // @todo: Simply use HWAddr directly once 2589 is implemented
-    Lease4Collection returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER));
-    EXPECT_EQ(0, returned.size());
+    // Database should not let us add one that is too big
+    // (The 42 is a random value put in each byte of the address.)
+    // @todo: 2589 will make this test impossible
+    leases[1]->hwaddr_.resize(HWAddr::MAX_HWADDR_LEN + 100, 42);
+    EXPECT_THROW(lmptr_->addLease(leases[1]), isc::dhcp::DbOperationError);
 }
 
 /// @brief Check GetLease4 methods - access by Hardware Address & Subnet ID
@@ -845,8 +841,9 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetId) {
     // Get the leases matching the hardware address of lease 1 and
     // subnet ID of lease 1.  Result should be a single lease - lease 1.
     // @todo: Simply use HWAddr directly once 2589 is implemented
-    Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
-                                           leases[1]->subnet_id_);
+    Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, 
+        HTYPE_ETHER), leases[1]->subnet_id_);
+
     ASSERT_TRUE(returned);
     detailCompareLease(leases[1], returned);
 
@@ -881,9 +878,8 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetId) {
     leases[1]->addr_ = leases[2]->addr_;
     EXPECT_TRUE(lmptr_->addLease(leases[1]));
     // @todo: Simply use HWAddr directly once 2589 is implemented
-    EXPECT_THROW(returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
-                                              leases[1]->subnet_id_),
-                 isc::dhcp::MultipleRecords);
+    EXPECT_THROW(returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, 
+        HTYPE_ETHER), leases[1]->subnet_id_), isc::dhcp::MultipleRecords);
 
     // Delete all leases in the database
     for (int i = 0; ADDRESS4[i] != NULL; ++i) {
@@ -903,28 +899,21 @@ TEST_F(MySqlLeaseMgrTest, getLease4HwaddrSubnetIdSize) {
 
     // Now add leases with increasing hardware address size and check
     // that they can be retrieved.
-    for (uint8_t i = 0; i <= Lease4::HWADDR_MAX; ++i) {
+    for (uint8_t i = 0; i <= HWAddr::MAX_HWADDR_LEN; ++i) {
         leases[1]->hwaddr_.resize(i, i);
         EXPECT_TRUE(lmptr_->addLease(leases[1]));
         // @todo: Simply use HWAddr directly once 2589 is implemented
-        Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
-                                               leases[1]->subnet_id_);
+        Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, 
+            HTYPE_ETHER), leases[1]->subnet_id_);
         ASSERT_TRUE(returned);
         detailCompareLease(leases[1], returned);
         (void) lmptr_->deleteLease(leases[1]->addr_);
     }
 
-    // Expect some error when getting a lease with too long a hardware
-    // address.  Set the contents of each byte to 42, a random value.
-    // @todo Check if there is some way of detecting that data added
-    //       to the database is truncated.  There does not appear to
-    //       be any indication in the C API.
-    leases[1]->hwaddr_.resize(Lease4::HWADDR_MAX + 100, 42);
-    EXPECT_TRUE(lmptr_->addLease(leases[1]));
-    // @todo: Simply use HWAddr directly once 2589 is implemented
-    Lease4Ptr returned = lmptr_->getLease4(HWAddr(leases[1]->hwaddr_, HTYPE_ETHER),
-                                           leases[1]->subnet_id_);
-    EXPECT_FALSE(returned);
+    // Database should not let us add one that is too big
+    // (The 42 is a random value put in each byte of the address.)
+    leases[1]->hwaddr_.resize(HWAddr::MAX_HWADDR_LEN + 100, 42);
+    EXPECT_THROW(lmptr_->addLease(leases[1]), isc::dhcp::DbOperationError);
 }
 
 /// @brief Check GetLease4 methods - access by Client ID