Commit a917e4ae authored by Thomas Markwalder's avatar Thomas Markwalder

[#900,!561] kea-dhcp4/6 now quietly drop empty or all-null string options

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)
parent fb19df73
1673. [bug] tmark
Fixed a bug introduced in Kea 1.6.0 (see #539) that caused
kea-dhcp4 and kea-dhcp6 to discard inbound packets containing
string options that consist solely of nulls. The servers
will now quiely omit empty or all-null string options from
inbound packets.
(Gitlab #900, !561 git TBD)
1672. [build] fdupont
Deprecated bind1st and bind2nd templates were replaced with
lambda expressions or plain bind templates.
......
......@@ -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;
......
......@@ -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)
......
......@@ -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;
......
......@@ -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.
......
......@@ -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.
......
......@@ -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");
}
......
......@@ -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::vector<uint8_t>nulls = { 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 };
......
......@@ -16,6 +16,7 @@
#include <dhcp/option_vendor.h>
#include <dhcp/pkt4.h>
#include <exceptions/exceptions.h>
#include <testutils/gtest_utils.h>
#include <util/buffer.h>
#include <util/encode/hex.h>
#include <pkt_captures.h>
......@@ -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<uint8_t> 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<OptionString>(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<OptionString>(opt));
EXPECT_EQ(3, opstr->getValue().length());
EXPECT_EQ("def", opstr->getValue());
}
} // end of anonymous namespace
......@@ -13,12 +13,14 @@
#include <dhcp/option6_ia.h>
#include <dhcp/option_int.h>
#include <dhcp/option_int_array.h>
#include <dhcp/option_string.h>
#include <dhcp/option_vendor.h>
#include <dhcp/iface_mgr.h>
#include <dhcp/pkt6.h>
#include <dhcp/hwaddr.h>
#include <dhcp/docsis3_option_defs.h>
#include <dhcp/tests/pkt_captures.h>
#include <testutils/gtest_utils.h>
#include <util/range_utilities.h>
#include <boost/bind.hpp>
#include <boost/date_time/posix_time/posix_time.hpp>
......@@ -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<OptionString>(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<OptionString>(opt));
EXPECT_EQ(3, opstr->getValue().length());
EXPECT_EQ("def", opstr->getValue());
}
}
......@@ -15,6 +15,7 @@
#include <dhcp/option_string.h>
#include <dhcp/option4_addrlst.h>
#include <dhcpsrv/cfg_option.h>
#include <testutils/gtest_utils.h>
#include <testutils/test_to_element.h>
#include <boost/foreach.hpp>
#include <boost/pointer_cast.hpp>
......@@ -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
......
......@@ -4,8 +4,8 @@
// 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_UTIL_H
#define GTEST_UTIL_H
#ifndef GTEST_UTILS_H
#define GTEST_UTILS_H
#include <gtest/gtest.h>
......@@ -54,7 +54,37 @@ namespace test {
} \
} \
/// @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_UTIL_H
#endif // GTEST_UTILS_H
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment