From: Marcin Siodelski <marcin@isc.org>
Date: Tue, 6 Sep 2016 10:28:03 +0000 (+0200)
Subject: [4252] CID 1364685. Unchecked dynamic_cast in Option::cloneInternal.
X-Git-Tag: trac5006_base~31^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1486d13c0d4bd46e65b47952add5677f87e24121;p=thirdparty%2Fkea.git

[4252] CID 1364685. Unchecked dynamic_cast in Option::cloneInternal.
---

diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h
index 6f53fcf843..3d0868c808 100644
--- a/src/lib/dhcp/option.h
+++ b/src/lib/dhcp/option.h
@@ -409,9 +409,11 @@ protected:
     /// be created.
     template<typename OptionType>
     OptionPtr cloneInternal() const {
-        boost::shared_ptr<OptionType>
-            option(new OptionType(*dynamic_cast<const OptionType*>(this)));
-        return (option);
+        const OptionType* cast_this = dynamic_cast<const OptionType*>(this);
+        if (cast_this) {
+            return (boost::shared_ptr<OptionType>(new OptionType(*cast_this)));
+        }
+        return (OptionPtr());
     }
 
     /// @brief Store option's header in a buffer.
diff --git a/src/lib/dhcp/tests/option_unittest.cc b/src/lib/dhcp/tests/option_unittest.cc
index 3d8289d47f..247a53d52a 100644
--- a/src/lib/dhcp/tests/option_unittest.cc
+++ b/src/lib/dhcp/tests/option_unittest.cc
@@ -9,6 +9,7 @@
 #include <dhcp/dhcp6.h>
 #include <dhcp/libdhcp++.h>
 #include <dhcp/option.h>
+#include <dhcp/option_int.h>
 #include <exceptions/exceptions.h>
 #include <util/buffer.h>
 
@@ -40,6 +41,7 @@ public:
     }
 
     using Option::unpackOptions;
+    using Option::cloneInternal;
 };
 
 class OptionTest : public ::testing::Test {
@@ -602,4 +604,18 @@ TEST_F(OptionTest, setEncapsulatedSpace) {
 
 }
 
+// This test verifies that cloneInternal returns NULL pointer if
+// non-compatible type is used as a template argument.
+// By non-compatible it is meant that the option instance doesn't
+// dynamic_cast to the type specified as template argument.
+// In our case, the NakedOption doesn't cast to OptionUint8 as the
+// latter is not derived from NakedOption.
+TEST_F(OptionTest, cloneInternal) {
+    NakedOption option;
+    OptionPtr clone;
+    // This shouldn't throw nor cause segmentation fault.
+    ASSERT_NO_THROW(clone = option.cloneInternal<OptionUint8>());
+    EXPECT_FALSE(clone);
+}
+
 }