From 88167106efd612d3b7b517e4af26e8a2f7b48713 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 6 Nov 2019 14:31:35 -0500 Subject: [PATCH] [#900,!561] kea-dhcp4/6 now quietly drop empty or all-null string options Cherry picked from master a917e4ae7e36e1273d3be5c370a1d3d705ded22b src/lib/dhcp/option.h class SkipThisOptionError - new exception type src/lib/dhcp/libdhcp++.cc LibDHCP::unpackOptions4() LibDHCP::unpackOptions6() - explicitly catches and handles SkipThisOptionError expceptions src/lib/dhcp/option_definition.cc OptionDefinition::optionFactory() - now rethrows SkipThisOptionError src/lib/dhcp/option_int.h OptionInt::unpack() - altered ambiguous exception text src/lib/dhcp/option_int_array.h OptionIntArray::unpack() - altered ambiguous exception text src/lib/dhcp/option_string.cc OptionString::unpack() - now throws SkipThisOptionError if option, once trimmed, is empty src/lib/dhcp/tests/option_string_unittest.cc Updated tests src/lib/dhcp/tests/pkt4_unittest.cc TEST_F(Pkt4Test, testSkipThisOptionError) - new test src/lib/dhcp/tests/pkt6_unittest.cc TEST_F(Pkt6Test, testSkipThisOptionError) - new test src/lib/dhcpsrv/tests/cfg_option_unittest.cc Updated expected exception text src/lib/testutils/gtest_utils.h Added two macros to emit exception info on throws. #define EXPECT_NO_THROW_LOG(statement) #define ASSERT_NO_THROW_LOG(statement) --- src/lib/dhcp/libdhcp++.cc | 48 +++++++---- src/lib/dhcp/option.h | 7 ++ src/lib/dhcp/option_definition.cc | 3 + src/lib/dhcp/option_int.h | 2 +- src/lib/dhcp/option_int_array.h | 2 +- src/lib/dhcp/option_string.cc | 2 +- src/lib/dhcp/tests/option_string_unittest.cc | 6 +- src/lib/dhcp/tests/pkt4_unittest.cc | 59 ++++++++++++- src/lib/dhcp/tests/pkt6_unittest.cc | 64 +++++++++++++- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 29 ++----- src/lib/testutils/gtest_utils.h | 90 ++++++++++++++++++++ 11 files changed, 264 insertions(+), 48 deletions(-) create mode 100644 src/lib/testutils/gtest_utils.h diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index a55313e618..44f06ed436 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -448,16 +448,24 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf, buf.begin() + offset, buf.begin() + offset + opt_len)); } else { - // The option definition has been found. Use it to create - // the option instance from the provided buffer chunk. - const OptionDefinitionPtr& def = *(range.first); - assert(def); - opt = def->optionFactory(Option::V6, opt_type, - buf.begin() + offset, - buf.begin() + offset + opt_len); + try { + // The option definition has been found. Use it to create + // the option instance from the provided buffer chunk. + const OptionDefinitionPtr& def = *(range.first); + assert(def); + opt = def->optionFactory(Option::V6, opt_type, + buf.begin() + offset, + buf.begin() + offset + opt_len); + } catch (const SkipThisOptionError& ex) { + opt.reset(); + } } + // add option to options - options.insert(std::make_pair(opt_type, opt)); + if (opt) { + options.insert(std::make_pair(opt_type, opt)); + } + offset += opt_len; } @@ -583,16 +591,24 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, buf.begin() + offset + opt_len)); opt->setEncapsulatedSpace(DHCP4_OPTION_SPACE); } else { - // The option definition has been found. Use it to create - // the option instance from the provided buffer chunk. - const OptionDefinitionPtr& def = *(range.first); - assert(def); - opt = def->optionFactory(Option::V4, opt_type, - buf.begin() + offset, - buf.begin() + offset + opt_len); + try { + // The option definition has been found. Use it to create + // the option instance from the provided buffer chunk. + const OptionDefinitionPtr& def = *(range.first); + assert(def); + opt = def->optionFactory(Option::V4, opt_type, + buf.begin() + offset, + buf.begin() + offset + opt_len); + } catch (const SkipThisOptionError& ex) { + opt.reset(); + } + } + + // If we have the option, insert it + if (opt) { + options.insert(std::make_pair(opt_type, opt)); } - options.insert(std::make_pair(opt_type, opt)); offset += opt_len; } last_offset = offset; diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h index fad71e5108..1d484f46e1 100644 --- a/src/lib/dhcp/option.h +++ b/src/lib/dhcp/option.h @@ -55,6 +55,13 @@ public: isc::Exception(file, line, what) { }; }; +class SkipThisOptionError : public Exception { +public: + SkipThisOptionError (const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) { }; +}; + + class Option { public: /// length of the usual DHCPv4 option header (there are exceptions) diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index c886e71222..1297e4a608 100644 --- a/src/lib/dhcp/option_definition.cc +++ b/src/lib/dhcp/option_definition.cc @@ -264,6 +264,9 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type, ; } return (OptionPtr(new OptionCustom(*this, u, begin, end))); + } catch (const SkipThisOptionError& ex) { + // We need to throw this one as is. + throw ex; } catch (const SkipRemainingOptionsError& ex) { // We need to throw this one as is. throw ex; diff --git a/src/lib/dhcp/option_int.h b/src/lib/dhcp/option_int.h index 05224307f1..dc2994661c 100644 --- a/src/lib/dhcp/option_int.h +++ b/src/lib/dhcp/option_int.h @@ -146,7 +146,7 @@ public: /// because it is checked in a constructor. virtual void unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { if (distance(begin, end) < sizeof(T)) { - isc_throw(OutOfRange, "Option " << getType() << " truncated"); + isc_throw(OutOfRange, "OptionInt " << getType() << " truncated"); } // @todo consider what to do if buffer is longer than data type. diff --git a/src/lib/dhcp/option_int_array.h b/src/lib/dhcp/option_int_array.h index ef379bb5c0..c72b1338ed 100644 --- a/src/lib/dhcp/option_int_array.h +++ b/src/lib/dhcp/option_int_array.h @@ -186,7 +186,7 @@ public: isc_throw(OutOfRange, "option " << getType() << " empty"); } if (distance(begin, end) % sizeof(T) != 0) { - isc_throw(OutOfRange, "option " << getType() << " truncated"); + isc_throw(OutOfRange, "OptionIntArray " << getType() << " truncated"); } // @todo consider what to do if buffer is longer than data type. diff --git a/src/lib/dhcp/option_string.cc b/src/lib/dhcp/option_string.cc index 68b80a40f0..64d4499350 100644 --- a/src/lib/dhcp/option_string.cc +++ b/src/lib/dhcp/option_string.cc @@ -88,7 +88,7 @@ OptionString::unpack(OptionBufferConstIter begin, // Trim off trailing null(s) end = util::str::seekTrimmed(begin, end, 0x0); if (std::distance(begin, end) == 0) { - isc_throw(isc::OutOfRange, "failed to parse an option '" + isc_throw(isc::dhcp::SkipThisOptionError, "failed to parse an option '" << getType() << "' holding string value" << "' was empty or contained only NULLs"); } diff --git a/src/lib/dhcp/tests/option_string_unittest.cc b/src/lib/dhcp/tests/option_string_unittest.cc index ae9346e1fc..0020a7878a 100644 --- a/src/lib/dhcp/tests/option_string_unittest.cc +++ b/src/lib/dhcp/tests/option_string_unittest.cc @@ -71,14 +71,14 @@ TEST_F(OptionStringTest, constructorFromBuffer) { // an exception. EXPECT_THROW( OptionString(Option::V4, 234, buf_.begin(), buf_.begin()), - isc::OutOfRange + isc::dhcp::SkipThisOptionError ); // NULLs should result in an exception. std::vectornulls = { 0, 0, 0 }; EXPECT_THROW( OptionString(Option::V4, 234, nulls.begin(), nulls.begin()), - isc::OutOfRange + isc::dhcp::SkipThisOptionError ); // Declare option as a scoped pointer here so as its scope is @@ -211,7 +211,7 @@ TEST_F(OptionStringTest, unpackNullsHandling) { // Only nulls should throw. OptionBuffer buffer = { 0, 0 }; - ASSERT_THROW(optv4.unpack(buffer.begin(), buffer.end()), isc::OutOfRange); + ASSERT_THROW(optv4.unpack(buffer.begin(), buffer.end()), isc::dhcp::SkipThisOptionError); // One trailing null should trim off. buffer = {'o', 'n', 'e', 0 }; diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc index 89c6142089..f33d1c001d 100644 --- a/src/lib/dhcp/tests/pkt4_unittest.cc +++ b/src/lib/dhcp/tests/pkt4_unittest.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -1180,7 +1181,7 @@ TEST_F(Pkt4Test, getType) { TEST_F(Pkt4Test, truncatedVendorLength) { // Build a good discover packet - Pkt4Ptr pkt = test::PktCaptures::discoverWithValidVIVSO(); + Pkt4Ptr pkt = dhcp::test::PktCaptures::discoverWithValidVIVSO(); // Unpacking should not throw ASSERT_NO_THROW(pkt->unpack()); @@ -1195,7 +1196,7 @@ TEST_F(Pkt4Test, truncatedVendorLength) { EXPECT_EQ(133+2, vivso->len()); // data + opt code + len // Build a bad discover packet - pkt = test::PktCaptures::discoverWithTruncatedVIVSO(); + pkt = dhcp::test::PktCaptures::discoverWithTruncatedVIVSO(); // Unpack should throw Skip exception ASSERT_THROW(pkt->unpack(), SkipRemainingOptionsError); @@ -1299,4 +1300,58 @@ TEST_F(Pkt4Test, nullTerminatedOptions) { EXPECT_EQ(0, memcmp(&packed[base_size], &packed_opts[0], packed_opts.size())); } +// Checks that unpacking correctly handles SkipThisOptionError by +// omitting the offending option from the unpacked options. +TEST_F(Pkt4Test, testSkipThisOptionError) { + vector orig = generateTestPacket2(); + + orig.push_back(0x63); + orig.push_back(0x82); + orig.push_back(0x53); + orig.push_back(0x63); + + orig.push_back(53); // Message Type + orig.push_back(1); // length=1 + orig.push_back(2); // type=2 + + orig.push_back(14); // merit-dump + orig.push_back(3); // length=3 + orig.push_back(0x61); // data="abc" + orig.push_back(0x62); + orig.push_back(0x63); + + orig.push_back(12); // Hostname + orig.push_back(3); // length=3 + orig.push_back(0); // data= all nulls + orig.push_back(0); + orig.push_back(0); + + orig.push_back(17); // root-path + orig.push_back(3); // length=3 + orig.push_back(0x64); // data="def" + orig.push_back(0x65); + orig.push_back(0x66); + + // Unpacking should not throw. + Pkt4Ptr pkt(new Pkt4(&orig[0], orig.size())); + ASSERT_NO_THROW_LOG(pkt->unpack()); + + // We should have option 14 = "abc". + OptionPtr opt; + OptionStringPtr opstr; + ASSERT_TRUE(opt = pkt->getOption(14)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("abc", opstr->getValue()); + + // We should not have option 12. + EXPECT_FALSE(opt = pkt->getOption(12)); + + // We should have option 17 = "def". + ASSERT_TRUE(opt = pkt->getOption(17)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("def", opstr->getValue()); +} + } // end of anonymous namespace diff --git a/src/lib/dhcp/tests/pkt6_unittest.cc b/src/lib/dhcp/tests/pkt6_unittest.cc index 042ffe8916..fefcf0b2b2 100644 --- a/src/lib/dhcp/tests/pkt6_unittest.cc +++ b/src/lib/dhcp/tests/pkt6_unittest.cc @@ -13,12 +13,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -1581,7 +1583,7 @@ TEST_F(Pkt6Test, getMACFromRemoteIdRelayOption) { // (Relay Supplied Options option). This test checks whether it was parsed // properly. See captureRelayed2xRSOO() description for details. TEST_F(Pkt6Test, rsoo) { - Pkt6Ptr msg = test::PktCaptures::captureRelayed2xRSOO(); + Pkt6Ptr msg = dhcp::test::PktCaptures::captureRelayed2xRSOO(); EXPECT_NO_THROW(msg->unpack()); @@ -1728,7 +1730,7 @@ TEST_F(Pkt6Test, getLabelEmptyClientId) { TEST_F(Pkt6Test, truncatedVendorLength) { // Build a good Solicit packet - Pkt6Ptr pkt = test::PktCaptures::captureSolicitWithVIVSO(); + Pkt6Ptr pkt = dhcp::test::PktCaptures::captureSolicitWithVIVSO(); // Unpacking should not throw ASSERT_NO_THROW(pkt->unpack()); @@ -1743,7 +1745,7 @@ TEST_F(Pkt6Test, truncatedVendorLength) { EXPECT_EQ(8, vivso->len()); // data + opt code + len // Build a bad Solicit packet - pkt = test::PktCaptures::captureSolicitWithTruncatedVIVSO(); + pkt = dhcp::test::PktCaptures::captureSolicitWithTruncatedVIVSO(); // Unpack should throw Skip exception ASSERT_THROW(pkt->unpack(), SkipRemainingOptionsError); @@ -1754,4 +1756,60 @@ TEST_F(Pkt6Test, truncatedVendorLength) { ASSERT_FALSE(x); } +// Checks that unpacking correctly handles SkipThisOptionError by +// omitting the offending option from the unpacked options. +TEST_F(Pkt6Test, testSkipThisOptionError) { + // Get a packet. We're really interested in its on-wire + // representation only. + Pkt6Ptr donor(capture1()); + + // That's our original content. It should be sane. + OptionBuffer orig = donor->data_; + + orig.push_back(0); + orig.push_back(41); // new-posix-timezone + orig.push_back(0); + orig.push_back(3); // length=3 + orig.push_back(0x61); // data="abc" + orig.push_back(0x62); + orig.push_back(0x63); + + orig.push_back(0); + orig.push_back(59); // bootfile-url + orig.push_back(0); + orig.push_back(3); // length=3 + orig.push_back(0); // data= all nulls + orig.push_back(0); + orig.push_back(0); + + orig.push_back(0); + orig.push_back(42); // new-tzdb-timezone + orig.push_back(0); + orig.push_back(3); // length=3 + orig.push_back(0x64); // data="def" + orig.push_back(0x65); + orig.push_back(0x66); + + // Unpacking should not throw. + Pkt6Ptr pkt(new Pkt6(&orig[0], orig.size())); + ASSERT_NO_THROW_LOG(pkt->unpack()); + + // We should have option 41 = "abc". + OptionPtr opt; + OptionStringPtr opstr; + ASSERT_TRUE(opt = pkt->getOption(41)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("abc", opstr->getValue()); + + // We should not have option 59. + EXPECT_FALSE(opt = pkt->getOption(59)); + + // We should have option 42 = "def". + ASSERT_TRUE(opt = pkt->getOption(42)); + ASSERT_TRUE(opstr = boost::dynamic_pointer_cast(opt)); + EXPECT_EQ(3, opstr->getValue().length()); + EXPECT_EQ("def", opstr->getValue()); +} + } diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 5c67bd9427..f2f8dc72be 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -552,16 +553,9 @@ TEST_F(CfgOptionTest, mergeInvalid) { // When we attempt to merge, it should fail, recognizing that // option 2, which has a formatted value, has no definition. - try { - this_cfg.merge(defs, other_cfg); - GTEST_FAIL() << "merge should have thrown"; - } catch (const InvalidOperation& ex) { - std::string exp_msg = "option: isc.2 has a formatted value: " - "'one,two,three' but no option definition"; - EXPECT_EQ(std::string(exp_msg), std::string(ex.what())); - } catch (const std::exception& ex) { - GTEST_FAIL() << "wrong exception thrown:" << ex.what(); - } + ASSERT_THROW_MSG(this_cfg.merge(defs, other_cfg), InvalidOperation, + "option: isc.2 has a formatted value: " + "'one,two,three' but no option definition"); // Now let's add an option definition that will force data truncated // error for option 1. @@ -569,17 +563,10 @@ TEST_F(CfgOptionTest, mergeInvalid) { // When we attempt to merge, it should fail because option 1's data // is not valid per its definition. - try { - this_cfg.merge(defs, other_cfg); - GTEST_FAIL() << "merge should have thrown"; - } catch (const InvalidOperation& ex) { - std::string exp_msg = "could not create option: isc.1" - " from data specified, reason:" - " Option 1 truncated"; - EXPECT_EQ(std::string(exp_msg), std::string(ex.what())); - } catch (const std::exception& ex) { - GTEST_FAIL() << "wrong exception thrown:" << ex.what(); - } + EXPECT_THROW_MSG(this_cfg.merge(defs, other_cfg), InvalidOperation, + "could not create option: isc.1" + " from data specified, reason:" + " OptionInt 1 truncated"); } // This test verifies the all of the valid option cases diff --git a/src/lib/testutils/gtest_utils.h b/src/lib/testutils/gtest_utils.h new file mode 100644 index 0000000000..13c1c0d122 --- /dev/null +++ b/src/lib/testutils/gtest_utils.h @@ -0,0 +1,90 @@ +// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#ifndef GTEST_UTILS_H +#define GTEST_UTILS_H + +#include + +namespace isc { +namespace test { + +/// @brief Verifies an expected exception type and message +/// +/// If the statement does not generate the expected exception +/// containing the expected message it will generate a non-fatal +/// failure. +/// +/// @param statement - statement block to execute +/// @param etype - type of exception expected +/// @param emsg - exact content expected to be returned by ex.what() +#define EXPECT_THROW_MSG(statement,etype,emsg) \ +{ \ + try { \ + statement; \ + ADD_FAILURE() << "no exception, expected: " << #etype; \ + } catch (const etype& ex) { \ + EXPECT_EQ(std::string(ex.what()), emsg); \ + } catch (...) { \ + ADD_FAILURE() << "wrong exception type thrown, expected: " << #etype; \ + } \ +} \ + +/// @brief Verifies an expected exception type and message +/// +/// If the statement does not generate the expected exception +/// containing the expected message it will generate a fatal +/// failure. +/// +/// @param statement - statement block to execute +/// @param etype - type of exception expected +/// @param emsg - exact content expected to be returned by ex.what() +#define ASSERT_THROW_MSG(statement,etype,emsg) \ +{ \ + try { \ + statement; \ + GTEST_FAIL() << "no exception, expected: " << #etype; \ + } catch (const etype& ex) { \ + ASSERT_EQ(std::string(ex.what()), emsg); \ + } catch (...) { \ + GTEST_FAIL() << "wrong exception type thrown, expected: " << #etype; \ + } \ +} \ + +/// @brief Adds a non-fatal failure with exception info, if the given +/// expression throws +/// +/// @param statement - statement block to execute +#define EXPECT_NO_THROW_LOG(statement) \ +{ \ + try { \ + statement; \ + } catch (const std::exception& ex) { \ + ADD_FAILURE() << #statement << "threw: " << ex.what(); \ + } catch (...) { \ + ADD_FAILURE() << #statement << "threw non-std::exception"; \ + } \ +} \ + +/// @brief Generates a fatal failure with exception info, if the given +/// expression throws +/// +/// @param statement - statement block to execute +#define ASSERT_NO_THROW_LOG(statement) \ +{ \ + try { \ + statement; \ + } catch (const std::exception& ex) { \ + GTEST_FAIL() << #statement << "threw: " << ex.what(); \ + } catch (...) { \ + GTEST_FAIL() << #statement << "threw non-std::exception"; \ + } \ +} \ + +}; // end of isc::test namespace +}; // end of isc namespace + +#endif // GTEST_UTILS_H -- 2.47.2