[#2021] Allow 0 length OpaqueDataTuple to be pack()ed

diff --git a/src/lib/dhcp/opaque_data_tuple.cc b/src/lib/dhcp/opaque_data_tuple.cc
index 724a466107c62e4ab46a457bdbade70fdd7c332a..a8a5750df3161b36018fbb4f0ac9cab226f99c47 100644 (file)
--- a/src/lib/dhcp/opaque_data_tuple.cc
+++ b/src/lib/dhcp/opaque_data_tuple.cc
@@ -51,10 +51,7 @@ OpaqueDataTuple::getText() const {
 
 void
 OpaqueDataTuple::pack(isc::util::OutputBuffer& buf) const {
-    if (getLength() == 0) {
-        isc_throw(OpaqueDataTupleError, "failed to create on-wire format of the"
-                  " opaque data field, because the field appears to be empty");
-    } else if ((1 << (getDataFieldSize() * 8)) <= getLength()) {
+    if ((1 << (getDataFieldSize() * 8)) <= getLength()) {
         isc_throw(OpaqueDataTupleError, "failed to create on-wire format of the"
                   " opaque data field, because current data length "
                   << getLength() << " exceeds the maximum size for the length"

diff --git a/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc b/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc
index fa3fa0f1ed241131a78ee96e3d3093ec6922f168..974a0bf27bbc97f07b39eff6cc647f12175b95c6 100644 (file)
--- a/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc
+++ b/src/lib/dhcp/tests/opaque_data_tuple_unittest.cc
@@ -270,17 +270,22 @@ TEST(OpaqueDataTuple, pack1Byte) {
     OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_1_BYTE);
     // Initially, the tuple should be empty.
     ASSERT_EQ(0, tuple.getLength());
-    // The empty data doesn't make much sense, so the pack() should not
-    // allow it.
+    // It turns out that Option 124 can be sent with 0 length Opaque Data
+    // See #2021 for more details
     OutputBuffer out_buf(10);
-    EXPECT_THROW(tuple.pack(out_buf), OpaqueDataTupleError);
+    ASSERT_NO_THROW(tuple.pack(out_buf));
+    ASSERT_EQ(1, out_buf.getLength());
+    const uint8_t* zero_len = static_cast<const uint8_t*>(out_buf.getData());
+    ASSERT_EQ(0, *zero_len);
+    // Reset the output buffer for another test.
+    out_buf.clear();
     // Set the data for tuple.
     std::vector<uint8_t> data;
     for (uint8_t i = 0; i < 100; ++i) {
         data.push_back(i);
     }
     tuple.assign(data.begin(), data.size());
-    // The pack should now succeed.
+    // Packing the data should succeed.
     ASSERT_NO_THROW(tuple.pack(out_buf));
     // The rendered buffer should be 101 bytes long - 1 byte for length,
     // 100 bytes for the actual data.
@@ -329,17 +334,22 @@ TEST(OpaqueDataTuple, pack2Bytes) {
     OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
     // Initially, the tuple should be empty.
     ASSERT_EQ(0, tuple.getLength());
-    // The empty data doesn't make much sense, so the pack() should not
-    // allow it.
+    // It turns out that Option 124 can be sent with 0 length Opaque Data
+    // See #2021 for more details
     OutputBuffer out_buf(10);
-    EXPECT_THROW(tuple.pack(out_buf), OpaqueDataTupleError);
+    ASSERT_NO_THROW(tuple.pack(out_buf));
+    ASSERT_EQ(2, out_buf.getLength());
+    const uint16_t* zero_len = static_cast<const uint16_t*>(out_buf.getData());
+    ASSERT_EQ(0, *zero_len);
+    // Reset the output buffer for another test.
+    out_buf.clear();
     // Set the data for tuple.
     std::vector<uint8_t> data;
     for (unsigned i = 0; i < 512; ++i) {
         data.push_back(i & 0xff);
     }
     tuple.assign(data.begin(), data.size());
-    // The pack should now succeed.
+    // Packing the data should succeed.
     ASSERT_NO_THROW(tuple.pack(out_buf));
     // The rendered buffer should be 514 bytes long - 2 bytes for length,
     // 512 bytes for the actual data.
@@ -403,6 +413,22 @@ TEST(OpaqueDataTuple, unpack1ByteZeroLength) {
     EXPECT_EQ(0, tuple.getLength());
 }
 
+// This test verifies that the tuple having a length of 0, followed by no
+// data, is decoded from the wire format.
+TEST(OpaqueDataTuple, unpack1ByteZeroLengthNoData) {
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_1_BYTE);
+    OpaqueDataTuple::Buffer wire_data = {0};
+    ASSERT_NO_THROW(tuple.unpack(wire_data.begin(), wire_data.end()));
+}
+
+// This test verifies that the tuple having a length of 0, followed by no
+// data, is decoded from the wire format.
+TEST(OpaqueDataTuple, unpack2ByteZeroLengthNoData) {
+    OpaqueDataTuple tuple(OpaqueDataTuple::LENGTH_2_BYTES);
+    OpaqueDataTuple::Buffer wire_data = {0, 0};
+    ASSERT_NO_THROW(tuple.unpack(wire_data.begin(), wire_data.end()));
+}
+
 // This test verifies that exception is thrown if the empty buffer is being
 // parsed.
 TEST(OpaqueDataTuple, unpack1ByteEmptyBuffer) {