[trac910] completed TSIG+TC support: added a minor case of too many questions.

src/lib/dns/message.cc

@@ -240,15 +240,20 @@ MessageImpl::toWire(AbstractMessageRenderer& renderer, TSIGContext* tsig_ctx) {
     }
 
     // Reserve the space for TSIG (if needed) so that we can handle truncation
-    // case correctly later when that happens.
+    // case correctly later when that happens.  orig_xxx variables remember
+    // some configured parameters of renderer in case they are needed in
+    // truncation processing below.
     const size_t tsig_len = (tsig_ctx != NULL) ? tsig_ctx->getTSIGLength() : 0;
+    const size_t orig_msg_len_limit = renderer.getLengthLimit();
+    const AbstractMessageRenderer::CompressMode orig_compress_mode =
+        renderer.getCompressMode();
     if (tsig_len > 0) {
-        if (tsig_len > renderer.getLengthLimit()) {
+        if (tsig_len > orig_msg_len_limit) {
             isc_throw(InvalidParameter, "Failed to render DNS message: "
                       "too small limit for a TSIG (" <<
-                      renderer.getLengthLimit() << ")");
+                      orig_msg_len_limit << ")");
         }
-        renderer.setLengthLimit(renderer.getLengthLimit() - tsig_len);
+        renderer.setLengthLimit(orig_msg_len_limit - tsig_len);
     }
 
     // reserve room for the header
@@ -302,14 +307,15 @@ MessageImpl::toWire(AbstractMessageRenderer& renderer, TSIGContext* tsig_ctx) {
 
     // If we're adding a TSIG to a truncated message, clear all RRsets
     // from the message except for the question before adding the TSIG.
-    // TODO: If even the question doesn't fit, don't include any question.
+    // If even (some of) the question doesn't fit, don't include it.
     if (tsig_ctx != NULL && renderer.isTruncated()) {
         renderer.clear();
+        renderer.setLengthLimit(orig_msg_len_limit - tsig_len);
+        renderer.setCompressMode(orig_compress_mode);
         renderer.skip(HEADERLEN);
-        qdcount =
-            for_each(questions_.begin(), questions_.end(),
-                     RenderSection<QuestionPtr>(renderer,
-                                                false)).getTotalCount();
+        qdcount = for_each(questions_.begin(), questions_.end(),
+                           RenderSection<QuestionPtr>(renderer,
+                                                      false)).getTotalCount();
         ancount = 0;
         nscount = 0;
         arcount = 0;
@@ -348,7 +354,7 @@ MessageImpl::toWire(AbstractMessageRenderer& renderer, TSIGContext* tsig_ctx) {
     // Add TSIG, if necessary, at the end of the message.
     if (tsig_ctx != NULL) {
         // Release the reserved space in the renderer.
-        renderer.setLengthLimit(renderer.getLengthLimit() + tsig_len);
+        renderer.setLengthLimit(orig_msg_len_limit);
 
         const int tsig_count =
             tsig_ctx->sign(qid_, renderer.getData(),

src/lib/dns/python/tests/message_python_test.py

@@ -356,6 +356,20 @@ class MessageTest(unittest.TestCase):
                                         [LONG_TXT1, LONG_TXT3])
         self.__common_tsig_checks("message_toWire4.wire")
 
+    def test_to_wire_tsig_truncation3(self):
+        self.r.set_opcode(Opcode.QUERY())
+        self.r.set_rcode(Rcode.NOERROR())
+        for i in range(1, 68):
+            self.r.add_question(Question(Name("www.example.com"),
+                                         RRClass("IN"), RRType(i)))
+        renderer = MessageRenderer()
+        self.r.to_wire(renderer, self.tsig_ctx)
+
+        self.p.from_wire(renderer.get_data())
+        self.assertTrue(self.p.get_header_flag(Message.HEADERFLAG_TC))
+        self.assertEqual(66, self.p.get_rr_count(Message.SECTION_QUESTION))
+        self.assertNotEqual(None, self.p.get_tsig_record())
+
     def test_to_wire_tsig_no_truncation(self):
         fix_current_time(0x4e17b38d)
         data = factoryFromFile(self.p, "message_fromWire18.wire")

src/lib/dns/tests/message_unittest.cc

@@ -772,6 +772,39 @@ TEST_F(MessageTest, toWireTSIGTruncation2) {
     }
 }
 
+TEST_F(MessageTest, toWireTSIGTruncation3) {
+    // Similar to previous ones, but truncation occurs due to too many
+    // Questions (very unusual, but not necessarily illegal).
+
+    // We are going to create a message starting with a standard
+    // header (12 bytes) and multiple questions in the Question
+    // section of the same owner name (changing the RRType, just so
+    // that it would be the form that would be accepted by the BIND 9
+    // parser).  The first Question is 21 bytes in length, and the subsequent
+    // ones are 6 bytes.  We'll also use a TSIG whose size is 85 bytes.
+    // Up to 66 questions can fit in the standard 512-byte buffer
+    // (12 + 21 + 6 * 65 + 85 = 508).  If we try to add one more it would
+    // result in truncation.
+    message_render.setOpcode(Opcode::QUERY());
+    message_render.setRcode(Rcode::NOERROR());
+    for (int i = 1; i <= 67; ++i) {
+        message_render.addQuestion(Question(Name("www.example.com"),
+                                            RRClass::IN(), RRType(i)));
+    }
+    message_render.toWire(renderer, tsig_ctx);
+
+    // Check the rendered data by parsing it.  We only check it has the
+    // TC bit on, has the correct number of questions, and has a TSIG RR.
+    // Checking the signature wouldn't be necessary for this rare case
+    // scenario.
+    InputBuffer buffer(renderer.getData(), renderer.getLength());
+    message_parse.fromWire(buffer);
+    EXPECT_TRUE(message_parse.getHeaderFlag(Message::HEADERFLAG_TC));
+    // Note that the number of questions are 66, not 67 as we tried to add.
+    EXPECT_EQ(66, message_parse.getRRCount(Message::SECTION_QUESTION));
+    EXPECT_TRUE(message_parse.getTSIGRecord() != NULL);
+}
+
 TEST_F(MessageTest, toWireTSIGNoTruncation) {
     // A boundary case that shouldn't cause truncation: the resulting
     // response message with a TSIG will be 512 bytes long.
@@ -795,20 +828,21 @@ TEST_F(MessageTest, toWireTSIGNoTruncation) {
 }
 
 // This is a buggy renderer for testing.  It behaves like the straightforward
-// MessageRenderer, but once its internal buffer reaches the length for
-// the header and a question for www.example.com (33 bytes), its
-// getLengthLimit() returns a faked value, which would make TSIG RR rendering
-// fail unexpectedly in the test that follows.
+// MessageRenderer, but once it has some data, its setLengthLimit() ignores
+// the given parameter and resets the limit to the current length, making
+// subsequent insertion result in truncation, which would make TSIG RR
+// rendering fail unexpectedly in the test that follows.
 class BadRenderer : public MessageRenderer {
 public:
     BadRenderer(isc::util::OutputBuffer& buffer) :
         MessageRenderer(buffer)
     {}
-    virtual size_t getLengthLimit() const {
-        if (MessageRenderer::getLength() >= 33) {
-            return (0);
+    virtual void setLengthLimit(size_t len) {
+        if (getLength() > 0) {
+            MessageRenderer::setLengthLimit(getLength());
+        } else {
+            MessageRenderer::setLengthLimit(len);
         }
-        return (MessageRenderer::getLengthLimit());
     }
 };