[2946] added a test case to reproduce the use-after-free situation

src/lib/asiodns/sync_udp_server.cc

@@ -26,6 +26,8 @@
 
 #include <boost/bind.hpp>
 
+#include <cassert>
+
 #include <sys/types.h>
 #include <netinet/in.h>
 #include <sys/socket.h>
@@ -75,13 +77,11 @@ SyncUDPServer::scheduleRead() {
 
 void
 SyncUDPServer::handleRead(const asio::error_code& ec, const size_t length) {
-    // If the server has been stopped, it could even have been destroyed
-    // by the time of this call.  We'll solve this problem in #2946, but
-    // until then we exit as soon as possible without accessing any other
-    // invalidated fields (note that referencing stopped_ is also incorrect,
-    // but experiments showed it often keeps the original value in practice,
-    // so we live with it until the complete fix).
     if (stopped_) {
+        // stopped_ can be set to true only after the socket object is closed.
+        // checking this would also detect premature destruction of 'this'
+        // object.
+        assert(socket_ && !socket_->is_open());
         return;
     }
     if (ec) {

src/lib/asiodns/sync_udp_server.h

@@ -133,7 +133,7 @@ private:
     // requires it.
     isc::dns::MessagePtr query_, answer_;
     // The socket used for the communication
-    std::auto_ptr<asio::ip::udp::socket> socket_;
+    boost::scoped_ptr<asio::ip::udp::socket> socket_;
     // Wrapper of socket_ in the form of asiolink::IOSocket.
     // "DummyIOCallback" is not necessary for this class, but using the
     // template is the easiest way to create a UDP instance of IOSocket.

src/lib/asiodns/tests/dns_server_unittest.cc

@@ -572,7 +572,6 @@ TYPED_TEST(DNSServerTest, stopUDPServerBeforeItStartServing) {
     EXPECT_TRUE(this->serverStopSucceed());
 }
 
-
 // Test whether udp server stopped successfully during message check.
 // This only works for non-sync server; SyncUDPServer doesn't use checkin
 // callback.
@@ -621,7 +620,6 @@ TYPED_TEST(DNSServerTest, stopUDPServerMoreThanOnce) {
     EXPECT_TRUE(this->serverStopSucceed());
 }
 
-
 TYPED_TEST(DNSServerTest, stopTCPServerAfterOneQuery) {
     this->testStopServerByStopper(*this->tcp_server_, this->tcp_client_,
                                   this->tcp_client_);
@@ -769,4 +767,27 @@ TEST_F(SyncServerTest, nullLookupCallback) {
                  isc::InvalidParameter);
 }
 
+TEST_F(SyncServerTest, resetUDPServerBeforeEvent) {
+    // Reset the UDP server object after starting and before it would get
+    // an event from io_service (in this case abort event).  The following
+    // sequence confirms it's shut down immediately, and without any
+    // disruption.
+
+    // Since we'll stop the server run() should immediately return, and
+    // it's very unlikely to cause hangup.  But we'll make very sure it
+    // doesn't happen.
+    const unsigned int IO_SERVICE_TIME_OUT = 5;
+    (*udp_server_)();
+    udp_server_->stop();
+    udp_server_.reset();
+    void (*prev_handler)(int) =
+                std::signal(SIGALRM, DNSServerTestBase::stopIOService);
+    current_service = &service;
+    alarm(IO_SERVICE_TIME_OUT);
+    service.run();
+    alarm(0);
+    std::signal(SIGALRM, prev_handler);
+    EXPECT_FALSE(io_service_is_time_out);
+}
+
 }