[trac812] ensure TSIGContext::sign() provides strong exception guarantee

added new test for that.
with some other cleanups.

src/lib/dns/tests/tsig_unittest.cc

@@ -14,6 +14,7 @@
 
 #include <time.h>
 #include <string>
+#include <stdexcept>
 #include <vector>
 
 #include <boost/scoped_ptr.hpp>
@@ -24,6 +25,7 @@
 
 #include <util/buffer.h>
 #include <util/encode/base64.h>
+#include <util/unittests/newhook.h>
 
 #include <dns/message.h>
 #include <dns/messagerenderer.h>
@@ -279,6 +281,40 @@ TEST_F(TSIGTest, signBadData) {
     EXPECT_THROW(tsig_ctx->sign(0, &dummy_data, 0), InvalidParameter);
 }
 
+#ifdef ENABLE_CUSTOM_OPERATOR_NEW
+// We enable this test only when we enable custom new/delete at build time
+// We could enable/disable the test runtime using the gtest filter, but
+// we'd basically like to minimize the number of disabled tests (they
+// should generally be considered tests that temporarily fail and should
+// be fixed).
+TEST_F(TSIGTest, signExceptionSafety) {
+    // Check sign() provides the strong exception guarantee for the simpler
+    // case (with a key error and empty MAC).  The general case is more
+    // complicated and involves more memory allocation, so the test result
+    // won't be reliable.
+
+    tsig_verify_ctx->verifyTentative(createMessageAndSign(qid, test_name,
+                                                          tsig_ctx.get()),
+                                     TSIGError::BAD_KEY());
+    // At this point the state should be changed to "CHECKED"
+    ASSERT_EQ(TSIGContext::CHECKED, tsig_verify_ctx->getState());
+    try {
+        int dummydata;
+        isc::util::unittests::force_throw_on_new = true;
+        isc::util::unittests::throw_size_on_new = sizeof(TSIGRecord);
+        tsig_verify_ctx->sign(0, &dummydata, sizeof(dummydata));
+        isc::util::unittests::force_throw_on_new = false;
+        ASSERT_FALSE(true) << "Expected throw on new, but it didn't happen";
+    } catch (const std::bad_alloc&) {
+        isc::util::unittests::force_throw_on_new = false;
+
+        // sign() threw, so the state should still be "CHECKED".
+        EXPECT_EQ(TSIGContext::CHECKED, tsig_verify_ctx->getState());
+    }
+    isc::util::unittests::force_throw_on_new = false;
+}
+#endif  // ENABLE_CUSTOM_OPERATOR_NEW
+
 // Same test as "sign" but use a different algorithm just to confirm we don't
 // naively hardcode constants specific to a particular algorithm.
 // Test data generated by

src/lib/dns/tsig.cc

@@ -19,7 +19,6 @@
 #include <cassert>              // for the tentative verifyTentative()
 #include <vector>
 
-#include <boost/lexical_cast.hpp>
 #include <boost/shared_ptr.hpp>
 
 #include <exceptions/exceptions.h>
@@ -67,6 +66,12 @@ gettimeofdayWrapper() {
 
 namespace {
 typedef boost::shared_ptr<HMAC> HMACPtr;
+}
+
+const RRClass&
+TSIGRecord::getClass() {
+    return (RRClass::ANY());
+}
 
 struct TSIGContext::TSIGContextImpl {
     TSIGContextImpl(const TSIGKey& key) :
@@ -79,12 +84,6 @@ struct TSIGContext::TSIGContextImpl {
     TSIGError error_;
     uint64_t previous_timesigned_; // only meaningful for response with BADTIME
 };
-}
-
-const RRClass&
-TSIGRecord::getClass() {
-    return (RRClass::ANY());
-}
 
 TSIGContext::TSIGContext(const TSIGKey& key) : impl_(new TSIGContextImpl(key))
 {
@@ -123,12 +122,12 @@ TSIGContext::sign(const uint16_t qid, const void* const data,
     // For errors related to key or MAC, return an unsigned response as
     // specified in Section 4.3 of RFC2845.
     if (error == TSIGError::BAD_SIG() || error == TSIGError::BAD_KEY()) {
-        impl_->previous_digest_.clear();
-        impl_->state_ = SIGNED;
         ConstTSIGRecordPtr tsig(new TSIGRecord(
                                     any::TSIG(impl_->key_.getAlgorithmName(),
                                               now, DEFAULT_FUDGE, NULL, 0,
                                               qid, error.getCode(), 0, NULL)));
+        impl_->previous_digest_.clear();
+        impl_->state_ = SIGNED;
         return (tsig);
     }
 
@@ -191,16 +190,17 @@ TSIGContext::sign(const uint16_t qid, const void* const data,
     const uint16_t otherlen = variables.getLength();
 
     // Get the final digest, update internal state, then finish.
-    impl_->previous_digest_ = hmac->sign();
-    impl_->state_ = SIGNED;
+    vector<uint8_t> digest = hmac->sign();
     ConstTSIGRecordPtr tsig(new TSIGRecord(
                                 any::TSIG(impl_->key_.getAlgorithmName(),
                                           time_signed, DEFAULT_FUDGE,
-                                          impl_->previous_digest_.size(),
-                                          &impl_->previous_digest_[0],
+                                          digest.size(), &digest[0],
                                           qid, error.getCode(), otherlen,
                                           otherlen == 0 ?
                                           NULL : variables.getData())));
+    // Exception free from now on.
+    impl_->previous_digest_.swap(digest);
+    impl_->state_ = SIGNED;
     return (tsig);
 }
 
@@ -222,30 +222,5 @@ TSIGContext::verifyTentative(ConstTSIGRecordPtr tsig, TSIGError error) {
 
     impl_->state_ = CHECKED;
 }
-
-namespace {
-const char* const tsigerror_text[] = {
-    "BADSIG",
-    "BADKEY",
-    "BADTIME"
-};
-}
-
-TSIGError::TSIGError(Rcode rcode) : code_(rcode.getCode()) {
-    if (code_ > MAX_RCODE_FOR_TSIGERROR) {
-        isc_throw(OutOfRange, "Invalid RCODE for TSIG Error: " << rcode);
-    }
-}
-
-string
-TSIGError::toText() const {
-    if (code_ <= MAX_RCODE_FOR_TSIGERROR) {
-        return (Rcode(code_).toText());
-    } else if (code_ <= BAD_TIME_CODE) {
-        return (tsigerror_text[code_ - (MAX_RCODE_FOR_TSIGERROR + 1)]);
-    } else {
-        return (boost::lexical_cast<string>(code_));
-    }
-}
 } // namespace dns
 } // namespace isc

src/lib/dns/tsig.h

@@ -106,22 +106,22 @@ typedef boost::shared_ptr<const TSIGRecord> ConstTSIGRecordPtr;
 /// This class supports all these cases.
 ///
 /// A \c TSIGContext object is generally constructed with a TSIG key to be
-/// used for the session, and keeps truck of various kinds of session specific
+/// used for the session, and keeps track of various kinds of session specific
 /// information, such as the original digest while waiting for a response or
 /// verification error information that is to be used for a subsequent
 /// response.
 ///
 /// This class has two main methods, \c sign() and \c verify().
 /// The \c sign() method signs given data (which is supposed to be a complete
-/// DNS message to be signed) using the TSIG key and other related information
-/// associated with the \c TSIGContext object.
+/// DNS message without the TSIG itself) using the TSIG key and other
+/// related information associated with the \c TSIGContext object.
 /// The \c verify() method verifies a given DNS message that contains a TSIG
 /// RR using the key and other internal information.
 ///
 /// In general, a DNS client that wants to send a signed query will construct
 /// a \c TSIGContext object with the TSIG key that the client is intending to
-/// use, and sign the query with the context.  The client keeps the context,
-/// and verifies the response with it.
+/// use, and sign the query with the context.  The client will keeps the
+/// context, and verify the response with it.
 ///
 /// On the other hand, a DNS server will construct a \c TSIGContext object
 /// with the information of the TSIG RR included in a query with a set of
@@ -134,7 +134,7 @@ typedef boost::shared_ptr<const TSIGRecord> ConstTSIGRecordPtr;
 ///
 /// When multiple messages belong to the same TSIG session, either side
 /// (signer or verifier) will keep using the same context.  It records
-/// the latest session state (such as the previous digest) so that continues
+/// the latest session state (such as the previous digest) so that repeated
 /// calls to \c sign() or \c verify() work correctly in terms of the TSIG
 /// protocol.
 ///
@@ -204,7 +204,8 @@ public:
     /// generally expected to be a complete, wire-format DNS message
     /// that doesn't contain a TSIG RR, based on the TSIG key and
     /// other context information of \c TSIGContext, and returns a
-    /// result in the form of an \c rdata::any::TSIG object.
+    /// result in the form of a (pointer object pointing to)
+    /// \c TSIGRecord object.
     ///
     /// The caller of this method will use the returned value to render a
     /// complete TSIG RR into the message that has been signed so that it
@@ -220,9 +221,9 @@ public:
     /// description), and doesn't inspect it in any way.  For example, it
     /// doesn't check whether the data length is sane for a valid DNS message.
     /// This is also the reason why this method takes the \c qid parameter,
-    /// which will be used as the original ID of the resulting \c TSIG object
-    /// (RR), even though this value should be stored in the first two octets
-    /// (in wire format) of the given data.
+    /// which will be used as the original ID of the resulting
+    /// \c TSIGRecordx object, even though this value should be stored in the
+    /// first two octets (in wire format) of the given data.
     ///
     /// \note This method still checks and rejects empty data (\c NULL pointer
     /// data or the specified data length is 0) in order to avoid catastrophic
@@ -230,13 +231,9 @@ public:
     /// for HMAC computation, but obviously it doesn't make sense for a DNS
     /// message.
     ///
-    /// This method can throw exceptions (see the list), but does not provide
-    /// the strong exception guarantee.  That is, if an exception is thrown,
-    /// the internal state of the \c TSIGContext object can be changed, in
-    /// which case it's unlikely that the context can be used for (re)signing
-    /// or (re)verifying subsequent messages any more.  If the caller wants
-    /// to catch the exception and try to recover from it, it must drop the
-    /// TSIG session and start a new session with a new context.
+    /// This method provides the strong exception guarantee; unless the method
+    /// returns (without an exception being thrown), the internal state of
+    /// the \c TSIGContext won't be modified.
     ///
     /// \exception InvalidParameter \c data is NULL or \c data_len is 0
     /// \exception cryptolink::LibraryError Some unexpected error in the
@@ -244,7 +241,7 @@ public:
     /// \exception std::bad_alloc Temporary resource allocation failure
     ///
     /// \param qid The QID to be as the value of the original ID field of
-    /// the resulting TSIG
+    /// the resulting TSIG record
     /// \param data Points to the wire-format data to be signed
     /// \param data_len The length of \c data in bytes
     ///

src/lib/dns/tsigerror.cc

@@ -13,11 +13,42 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <ostream>
+#include <string>
 
+#include <boost/lexical_cast.hpp>
+
+#include <exceptions/exceptions.h>
+
+#include <dns/rcode.h>
 #include <dns/tsigerror.h>
 
 namespace isc {
 namespace dns {
+namespace {
+const char* const tsigerror_text[] = {
+    "BADSIG",
+    "BADKEY",
+    "BADTIME"
+};
+}
+
+TSIGError::TSIGError(Rcode rcode) : code_(rcode.getCode()) {
+    if (code_ > MAX_RCODE_FOR_TSIGERROR) {
+        isc_throw(OutOfRange, "Invalid RCODE for TSIG Error: " << rcode);
+    }
+}
+
+std::string
+TSIGError::toText() const {
+    if (code_ <= MAX_RCODE_FOR_TSIGERROR) {
+        return (Rcode(code_).toText());
+    } else if (code_ <= BAD_TIME_CODE) {
+        return (tsigerror_text[code_ - (MAX_RCODE_FOR_TSIGERROR + 1)]);
+    } else {
+        return (boost::lexical_cast<std::string>(code_));
+    }
+}
+
 std::ostream&
 operator<<(std::ostream& os, const TSIGError& error) {
     return (os << error.toText());

src/lib/util/unittests/Makefile.am

@@ -3,5 +3,6 @@ AM_CXXFLAGS = $(B10_CXXFLAGS)
 
 lib_LTLIBRARIES = libutil_unittests.la
 libutil_unittests_la_SOURCES = fork.h fork.cc
+libutil_unittests_la_SOURCES += newhook.h newhook.cc
 
 CLEANFILES = *.gcno *.gcda

src/lib/util/unittests/newhook.cc

+// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <stdlib.h>
+
+#include <new>
+#include <stdexcept>
+
+#include "newhook.h"
+
+#ifdef ENABLE_CUSTOM_OPERATOR_NEW
+void*
+operator new(size_t size) throw(std::bad_alloc) {
+    if (isc::util::unittests::force_throw_on_new &&
+        size == isc::util::unittests::throw_size_on_new) {
+        throw std::bad_alloc();
+    }
+    void* p = malloc(size);
+    if (p == NULL) {
+        throw std::bad_alloc();
+    }
+    return (p);
+}
+
+void
+operator delete(void* p) throw() {
+    if (p != NULL) {
+        free (p);
+    }
+}
+#endif
+
+namespace isc {
+namespace util {
+namespace unittests {
+bool force_throw_on_new = false;
+size_t throw_size_on_new = 0;
+}
+}
+}

src/lib/util/unittests/newhook.h

+// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef __UTIL_UNITTESTS_NEWHOOK_H
+#define __UTIL_UNITTESTS_NEWHOOK_H 1
+
+/**
+ * @file newhook.h
+ * @short Enable the use of special operator new that throws for testing.
+ *
+ * This small utility allows a test case to force the global operator new
+ * to throw for a given size to test a case where memory allocation fails
+ * (which normally doesn't happen).  To enable the feature, everything must
+ * be built with defining ENABLE_CUSTOM_OPERATOR_NEW beforehand, and set
+ * \c force_throw_on_new to \c true and \c throw_size_on_new to the size
+ * of data that should trigger the exception, immediately before starting
+ * the specific test that needs the exception.
+ *
+ * Example:
+ * \code #include <util/unittests/newhook.h>
+ * ...
+ * TEST(SomeTest, newException) {
+ *        isc::util::unittests::force_throw_on_new = true;
+ *        isc::util::unittests::throw_size_on_new = sizeof(Foo);
+ *        try {
+ *            // this will do 'new Foo()' internally and should throw
+ *            createFoo();
+ *            isc::util::unittests::force_throw_on_new = false;
+ *            ASSERT_FALSE(true) << "Expected throw on new";
+ *        } catch (const std::bad_alloc&) {
+ *            isc::util::unittests::force_throw_on_new = false;
+ *            // do some integrity check, etc, if necessary
+ *        }
+ * } \endcode
+ *
+ * Replacing the global operator new (and delete) is a dangerous technique,
+ * and triggering an exception solely based on the allocation size is not
+ * reliable, so this feature is disabled by default two-fold: The
+ * ENABLE_CUSTOM_OPERATOR_NEW build time variable, and run-time
+ * \c force_throw_on_new.
+ */
+
+namespace isc {
+namespace util {
+namespace unittests {
+/// Switch to enable the use of special operator new
+///
+/// This is set to \c false by default.
+extern bool force_throw_on_new;
+
+/// The allocation size that triggers an exception in the special operator new
+///
+/// The default value is 0.  The value of this variable has no meaning
+/// unless the use of the special operator is enabled at build time and
+/// via \c force_throw_on_new.
+extern size_t throw_size_on_new;
+}
+}
+}
+
+#endif // __UTIL_UNITTESTS_NEWHOOK_H
+
+// Local Variables:
+// mode: c++
+// End: