fixed the trailing garbage bug: b10-auth's MessageAnswer() shouldn't do anything because it builds everything in the process methods.

test cases were added for the bug, too.


git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac462@4070 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/auth/auth_srv.cc

@@ -162,33 +162,20 @@ private:
     AuthSrv* server_;
 };
 
-// This is a derived class of \c DNSAnswer, to serve as a
-// callback in the asiolink module.  It takes a completed
-// set of answer data from the DNS lookup and assembles it
-// into a wire-format response.
+// This is a derived class of \c DNSAnswer, to serve as a callback in the
+// asiolink module.  We actually shouldn't do anything in this class because
+// we build complete response messages in the process methods; otherwise
+// the response message will contain trailing garbage.  In future, we should
+// probably even drop the reliance on DNSAnswer.  We don't need the coroutine
+// tricks provided in that framework, and its overhead would be significant
+// in terms of performance consideration for the authoritative server
+// implementation.
 class MessageAnswer : public DNSAnswer {
 public:
-    MessageAnswer(AuthSrv* srv) : server_(srv) {}
-    virtual void operator()(const IOMessage& io_message, MessagePtr message,
-                            OutputBufferPtr buffer) const
-    {
-        MessageRenderer renderer(*buffer);
-        if (io_message.getSocket().getProtocol() == IPPROTO_UDP) {
-            ConstEDNSPtr edns(message->getEDNS());
-            renderer.setLengthLimit(edns ? edns->getUDPSize() :
-                Message::DEFAULT_MAX_UDPSIZE);
-        } else {
-            renderer.setLengthLimit(65535);
-        }
-        message->toWire(renderer);
-        if (server_->getVerbose()) {
-            cerr << "[b10-auth] sending a response (" << renderer.getLength()
-                 << " bytes):\n" << message->toText() << endl;
-        }
-    }
-
-private:
-    AuthSrv* server_;
+    MessageAnswer(AuthSrv*) {}
+    virtual void operator()(const IOMessage&, MessagePtr,
+                            OutputBufferPtr) const
+    {}
 };
 
 // This is a derived class of \c SimpleCallback, to serve

src/bin/auth/tests/auth_srv_unittest.cc

@@ -15,6 +15,19 @@
 // $Id$
 
 #include <config.h>
+
+#include <vector>
+
+#include <gtest/gtest.h>
+
+#include <dns/message.h>
+#include <dns/messagerenderer.h>
+#include <dns/name.h>
+#include <dns/rrclass.h>
+#include <dns/rrtype.h>
+#include <dns/rrttl.h>
+#include <dns/rdataclass.h>
+
 #include <datasrc/memory_datasrc.h>
 #include <auth/auth_srv.h>
 #include <testutils/srv_unittest.h>
@@ -22,6 +35,7 @@
 
 using namespace isc::cc;
 using namespace isc::dns;
+using namespace isc::dns::rdata;
 using namespace isc::data;
 using namespace isc::xfr;
 using namespace asiolink;
@@ -45,8 +59,104 @@ protected:
     MockXfroutClient xfrout;
     AuthSrv server;
     const RRClass rrclass;
+    vector<uint8_t> response_data;
 };
 
+// A helper function that builds a response to version.bind/TXT/CH that
+// should be identical to the response from our builtin (static) data source
+// by default.  The resulting wire-format data will be stored in 'data'.
+void
+createBuiltinVersionResponse(const qid_t qid, vector<uint8_t>& data) {
+    const Name version_name("version.bind");
+    Message message(Message::RENDER);
+
+    UnitTestUtil::createRequestMessage(message, Opcode::QUERY(),
+                                       qid, version_name,
+                                       RRClass::CH(), RRType::TXT());
+    message.setHeaderFlag(Message::HEADERFLAG_QR);
+    message.setHeaderFlag(Message::HEADERFLAG_AA);
+    RRsetPtr rrset_version = RRsetPtr(new RRset(version_name, RRClass::CH(),
+                                                RRType::TXT(), RRTTL(0)));
+    rrset_version->addRdata(generic::TXT(PACKAGE_STRING));
+    message.addRRset(Message::SECTION_ANSWER, rrset_version);
+
+    RRsetPtr rrset_version_ns = RRsetPtr(new RRset(version_name, RRClass::CH(),
+                                                   RRType::NS(), RRTTL(0)));
+    rrset_version_ns->addRdata(generic::NS(version_name));
+    message.addRRset(Message::SECTION_AUTHORITY, rrset_version_ns);
+
+    OutputBuffer obuffer(0);
+    MessageRenderer renderer(obuffer);
+    message.toWire(renderer);
+
+    data.clear();
+    data.assign(static_cast<const uint8_t*>(renderer.getData()),
+                static_cast<const uint8_t*>(renderer.getData()) +
+                renderer.getLength());
+}
+
+// In the following tests we confirm the response data is rendered in
+// wire format in the expected way.
+
+// The most primitive check: checking the result of the processMessage()
+// method
+TEST_F(AuthSrvTest, builtInQuery) {
+    UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(),
+                                       default_qid, Name("version.bind"),
+                                       RRClass::CH(), RRType::TXT());
+    createRequestPacket(request_message, IPPROTO_UDP);
+    server.processMessage(*io_message, parse_message, response_obuffer,
+                          &dnsserv);
+    createBuiltinVersionResponse(default_qid, response_data);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        response_obuffer->getData(),
+                        response_obuffer->getLength(),
+                        &response_data[0], response_data.size());
+}
+
+// Same test emulating the UDPServer class behavior (defined in libasiolink).
+// This is not a good test in that it assumes internal implementation details
+// of UDPServer, but we've encountered a regression due to the introduction
+// of that class, so we add a test for that case to prevent such a regression
+// in future.
+// Besides, the generalization of UDPServer is probably too much for the
+// authoritative only server in terms of performance, and it's quite likely
+// we need to drop it for the authoritative server implementation.
+// At that point we can drop this test, too.
+TEST_F(AuthSrvTest, builtInQueryViaDNSServer) {
+    UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(),
+                                       default_qid, Name("version.bind"),
+                                       RRClass::CH(), RRType::TXT());
+    createRequestPacket(request_message, IPPROTO_UDP);
+
+    (*server.getDNSLookupProvider())(*io_message, parse_message,
+                                     response_obuffer, &dnsserv);
+    (*server.getDNSAnswerProvider())(*io_message, parse_message,
+                                     response_obuffer);
+
+    createBuiltinVersionResponse(default_qid, response_data);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        response_obuffer->getData(),
+                        response_obuffer->getLength(),
+                        &response_data[0], response_data.size());
+}
+
+// Same type of test as builtInQueryViaDNSServer but for an error response.
+TEST_F(AuthSrvTest, iqueryViaDNSServer) {
+    createDataFromFile("iquery_fromWire.wire");
+    (*server.getDNSLookupProvider())(*io_message, parse_message,
+                                     response_obuffer, &dnsserv);
+    (*server.getDNSAnswerProvider())(*io_message, parse_message,
+                                     response_obuffer);
+
+    UnitTestUtil::readWireData("iquery_response_fromWire.wire",
+                               response_data);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData,
+                        response_obuffer->getData(),
+                        response_obuffer->getLength(),
+                        &response_data[0], response_data.size());
+}
+
 // Unsupported requests.  Should result in NOTIMP.
 TEST_F(AuthSrvTest, unsupportedRequest) {
     UNSUPPORTED_REQUEST_TEST;