[#1858] always check if psid is valid according to psid_len

diff --git a/src/lib/dhcp/option_data_types.cc b/src/lib/dhcp/option_data_types.cc
index 6f1bd04300cfcfeb690240681c88d74e916cecfb..a274d3fd520f61f0ff62bed98c6e82522443010e 100644 (file)
--- a/src/lib/dhcp/option_data_types.cc
+++ b/src/lib/dhcp/option_data_types.cc
@@ -522,15 +522,13 @@ OptionDataTypeUtil::readPsid(const std::vector<uint8_t>& buf) {
 
     // We need to check that the PSID value does not exceed the maximum value
     // for a specified PSID length. That means that all bits placed further than
-    // psid_len from the left must be set to 0. So, we create a bit mask
-    // by shifting a value of 0xFFFF to the left and right by psid_len. This
-    // leaves us with psid_len leftmost bits unset and the rest set. Next, we
-    // apply the mask on the PSID value from the buffer and make sure the result
-    // is 0. Otherwise, it means that there are some bits set in the PSID which
-    // aren't supposed to be set.
-    if ((psid_len > 0) &&
-        ((psid & static_cast<uint16_t>(static_cast<uint16_t>(0xFFFF << psid_len)
-                                       >> psid_len)) != 0)) {
+    // psid_len from the left must be set to 0. So, we create a bit mask by
+    // shifting a value of 0xFFFF to the right by psid_len. This leaves us with
+    // psid_len leftmost bits unset and the rest set. Next, we apply the mask on
+    // the PSID value from the buffer and make sure the result is 0. Otherwise,
+    // it means that there are some bits set in the PSID which aren't supposed
+    // to be set.
+    if ((psid_len > 0) && ((psid & (0xFFFFU >> psid_len)) != 0)) {
         isc_throw(BadDataTypeCast, "invalid PSID value " << psid
                   << " for a specified PSID length "
                   << static_cast<unsigned>(psid_len));
@@ -538,11 +536,11 @@ OptionDataTypeUtil::readPsid(const std::vector<uint8_t>& buf) {
 
     // All is good, so we can convert the PSID value read from the buffer to
     // the port set number.
-    if (psid_len == sizeof(psid) * 8) {
+    if (psid_len == 0) {
         // Shift by 16 always gives zero (CID 1398333)
         psid = 0;
     } else {
-        psid = psid >> (sizeof(psid) * 8 - psid_len);
+        psid >>= (sizeof(psid) * 8 - psid_len);
     }
     return (std::make_pair(PSIDLen(psid_len), PSID(psid)));
 }
@@ -556,7 +554,7 @@ OptionDataTypeUtil::writePsid(const PSIDLen& psid_len, const PSID& psid,
                   << ", this value is expected to be in range of 0 to 16");
     }
 
-    if (psid_len.asUint8() > 0 &&
+    if ((psid_len.asUint8() > 0) &&
         (psid.asUint16() > (0xFFFF >> (sizeof(uint16_t) * 8 - psid_len.asUint8())))) {
         isc_throw(BadDataTypeCast, "invalid PSID value " << psid.asUint16()
                   << " for a specified PSID length "
@@ -570,7 +568,6 @@ OptionDataTypeUtil::writePsid(const PSIDLen& psid_len, const PSID& psid,
                            &buf[buf.size() - 2], 2);
 }
 
-
 std::string
 OptionDataTypeUtil::readString(const std::vector<uint8_t>& buf) {
     std::string value;

diff --git a/src/lib/dhcp/tests/option_custom_unittest.cc b/src/lib/dhcp/tests/option_custom_unittest.cc
index 33bdc5e094a2e52691f779cb6e05fd02b632f890..ffbc565f1d095d4bbb20ce78a16d11152df3410c 100644 (file)
--- a/src/lib/dhcp/tests/option_custom_unittest.cc
+++ b/src/lib/dhcp/tests/option_custom_unittest.cc
@@ -679,7 +679,7 @@ TEST_F(OptionCustomTest, psidData) {
 
     // The PSID comprises a PSID length and PSID value.
     EXPECT_EQ(4, psid.first.asUnsigned());
-    EXPECT_EQ(0x08, psid.second.asUint16());
+    EXPECT_EQ(0x8, psid.second.asUint16());
 
     // Parsed option should have one suboption.
     EXPECT_TRUE(hasV6Suboption(option.get()));
@@ -1058,17 +1058,17 @@ TEST_F(OptionCustomTest, psidDataArray) {
     ASSERT_NO_THROW(psid1 = option->readPsid(1));
     ASSERT_NO_THROW(psid2 = option->readPsid(2));
 
-    // PSID value is equal to '1000b' (8).
+    // PSID value is equal to '1000b' (0x8).
     EXPECT_EQ(4, psid0.first.asUnsigned());
-    EXPECT_EQ(0x08, psid0.second.asUint16());
+    EXPECT_EQ(0x8, psid0.second.asUint16());
 
-    // PSID value is equal to '110101b' (0x35)
+    // PSID value is equal to '110101b' (0x35).
     EXPECT_EQ(6, psid1.first.asUnsigned());
     EXPECT_EQ(0x35, psid1.second.asUint16());
 
-    // PSID value is equal to '1b' (1).
+    // PSID value is equal to '1b' (0x1).
     EXPECT_EQ(1, psid2.first.asUnsigned());
-    EXPECT_EQ(0x01, psid2.second.asUint16());
+    EXPECT_EQ(0x1, psid2.second.asUint16());
 }
 
 // The purpose of this test is to verify that the data from a buffer
@@ -1236,10 +1236,10 @@ TEST_F(OptionCustomTest, recordData) {
     writeAddress(IOAddress("192.168.0.1"), buf);
     // Initialize field 4 to IPv6 address.
     writeAddress(IOAddress("2001:db8:1::1"), buf);
-    // Initialize PSID len and PSID value.
+    // Initialize field 5 PSID len and PSID value.
     writeInt<uint8_t>(6, buf);
     writeInt<uint16_t>(0xD400, buf);
-    // Initialize field 5 to string value.
+    // Initialize field 6 to string value.
     writeString("ABCD", buf);
 
     boost::scoped_ptr<OptionCustom> option;
@@ -1321,7 +1321,7 @@ TEST_F(OptionCustomTest, recordArrayData) {
     writeAddress(IOAddress("192.168.0.1"), buf);
     // Initialize field 4 to IPv6 address.
     writeAddress(IOAddress("2001:db8:1::1"), buf);
-    // Initialize PSID len and PSID value.
+    // Initialize field 5 PSID len and PSID value.
     writeInt<uint8_t>(6, buf);
     writeInt<uint16_t>(0xD400, buf);
     // Initialize last field 6 to a pair of int 12345678 and 87654321.
@@ -1615,7 +1615,7 @@ TEST_F(OptionCustomTest, setPsidData) {
     PSIDTuple psid;
     ASSERT_NO_THROW(psid = option->readPsid());
     EXPECT_EQ(0, psid.first.asUnsigned());
-    EXPECT_EQ(0, psid.second.asUint16());
+    EXPECT_EQ(0x0, psid.second.asUint16());
 
     // Write PSID.
     ASSERT_NO_THROW(option->writePsid(PSIDLen(4), PSID(8)));
@@ -1623,7 +1623,7 @@ TEST_F(OptionCustomTest, setPsidData) {
     // Read PSID back and make sure it is the one we just set.
     ASSERT_NO_THROW(psid = option->readPsid());
     EXPECT_EQ(4, psid.first.asUnsigned());
-    EXPECT_EQ(8, psid.second.asUint16());
+    EXPECT_EQ(0x8, psid.second.asUint16());
 }
 
 // The purpose of this test is to verify that an option comprising
@@ -1861,19 +1861,19 @@ TEST_F(OptionCustomTest, setPSIDPrefixArray) {
     ASSERT_NO_THROW({
          PSIDTuple psid0 = option->readPsid(0);
          EXPECT_EQ(4, psid0.first.asUnsigned());
-         EXPECT_EQ(1, psid0.second.asUint16());
+         EXPECT_EQ(0x1, psid0.second.asUint16());
     });
 
     ASSERT_NO_THROW({
          PSIDTuple psid1 = option->readPsid(1);
          EXPECT_EQ(0, psid1.first.asUnsigned());
-         EXPECT_EQ(0, psid1.second.asUint16());
+         EXPECT_EQ(0x0, psid1.second.asUint16());
     });
 
     ASSERT_NO_THROW({
          PSIDTuple psid2 = option->readPsid(2);
          EXPECT_EQ(1, psid2.first.asUnsigned());
-         EXPECT_EQ(1, psid2.second.asUint16());
+         EXPECT_EQ(0x1, psid2.second.asUint16());
     });
 }
 
@@ -2060,7 +2060,7 @@ TEST_F(OptionCustomTest, setRecordData) {
     PSIDTuple value5;
     ASSERT_NO_THROW(value5 = option->readPsid(5));
     EXPECT_EQ(0, value5.first.asUnsigned());
-    EXPECT_EQ(0, value5.second.asUint16());
+    EXPECT_EQ(0x0, value5.second.asUint16());
     PrefixTuple value6(ZERO_PREFIX_TUPLE);
     ASSERT_NO_THROW(value6 = option->readPrefix(6));
     EXPECT_EQ(0, value6.first.asUnsigned());
@@ -2097,7 +2097,7 @@ TEST_F(OptionCustomTest, setRecordData) {
     EXPECT_EQ("2001:db8:1::100", value4.toText());
     ASSERT_NO_THROW(value5 = option->readPsid(5));
     EXPECT_EQ(4, value5.first.asUnsigned());
-    EXPECT_EQ(8, value5.second.asUint16());
+    EXPECT_EQ(0x8, value5.second.asUint16());
     ASSERT_NO_THROW(value6 = option->readPrefix(6));
     EXPECT_EQ(48, value6.first.asUnsigned());
     EXPECT_EQ("2001:db8:1::", value6.second.toText());
@@ -2151,7 +2151,7 @@ TEST_F(OptionCustomTest, setRecordArrayData) {
     PSIDTuple value5;
     ASSERT_NO_THROW(value5 = option->readPsid(5));
     EXPECT_EQ(0, value5.first.asUnsigned());
-    EXPECT_EQ(0, value5.second.asUint16());
+    EXPECT_EQ(0x0, value5.second.asUint16());
     PrefixTuple value6(ZERO_PREFIX_TUPLE);
     ASSERT_NO_THROW(value6 = option->readPrefix(6));
     EXPECT_EQ(0, value6.first.asUnsigned());
@@ -2191,7 +2191,7 @@ TEST_F(OptionCustomTest, setRecordArrayData) {
     EXPECT_EQ("2001:db8:1::100", value4.toText());
     ASSERT_NO_THROW(value5 = option->readPsid(5));
     EXPECT_EQ(4, value5.first.asUnsigned());
-    EXPECT_EQ(8, value5.second.asUint16());
+    EXPECT_EQ(0x8, value5.second.asUint16());
     ASSERT_NO_THROW(value6 = option->readPrefix(6));
     EXPECT_EQ(48, value6.first.asUnsigned());
     EXPECT_EQ("2001:db8:1::", value6.second.toText());

diff --git a/src/lib/dhcp/tests/option_data_types_unittest.cc b/src/lib/dhcp/tests/option_data_types_unittest.cc
index 3c0ef5c0e2a248497caac359182e67d8dcec5fbd..58bac71a32fbda757cd1dac8f9380db211563b35 100644 (file)
--- a/src/lib/dhcp/tests/option_data_types_unittest.cc
+++ b/src/lib/dhcp/tests/option_data_types_unittest.cc
@@ -750,6 +750,18 @@ TEST_F(OptionDataTypesTest, readPsid) {
 
     buf.clear();
 
+    // PSID length is 16 (bits)
+    writeInt<uint8_t>(16, buf);
+    // 0xF000 is represented as 1111000000000000b, which is equivalent
+    // of portset 0xF000.
+    writeInt<uint16_t>(0xF000, buf);
+
+    ASSERT_NO_THROW(psid = OptionDataTypeUtil::readPsid(buf));
+    EXPECT_EQ(16, psid.first.asUnsigned());
+    EXPECT_EQ(0xF000, psid.second.asUint16());
+
+    buf.clear();
+
     // PSID length is 0, in which case PSID should be ignored.
     writeInt<uint8_t>(0, buf);
     // Let's put some junk into the PSID field to make sure it will