From: Dan Theisen <djt@isc.org>
Date: Fri, 5 Nov 2021 13:03:12 +0000 (-0700)
Subject: [#2021] Allow 0 length OpaqueDataTuple to be pack()ed
X-Git-Tag: Kea-2.1.1~6
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8d8b9bcd5a6975b3d05dcaa9465ef2cbd6a74049;p=thirdparty%2Fkea.git

[#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 724a466107..a8a5750df3 100644
--- 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 fa3fa0f1ed..974a0bf27b 100644
--- 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) {