[2946] mix enable_shared_from_this in SyncUDPServer to prevent use-after-free.

to enforce this style of creation, introduce a static factory method and
hide the constructor as private.

src/lib/asiodns/dns_service.cc

@@ -64,8 +64,9 @@ public:
     // SyncUDPServer has different constructor signature so it cannot be
     // templated.
     void addSyncUDPServerFromFD(int fd, int af) {
-        SyncUDPServerPtr server(new SyncUDPServer(io_service_.get_io_service(),
-                                                  fd, af, lookup_));
+        SyncUDPServerPtr server(SyncUDPServer::create(
+                                    io_service_.get_io_service(), fd, af,
+                                    lookup_));
         startServer(server);
     }
 

src/lib/asiodns/sync_udp_server.cc

@@ -40,13 +40,19 @@ using namespace isc::asiolink;
 namespace isc {
 namespace asiodns {
 
+SyncUDPServerPtr
+SyncUDPServer::create(asio::io_service& io_service, const int fd,
+                      const int af, DNSLookup* lookup)
+{
+    return (SyncUDPServerPtr(new SyncUDPServer(io_service, fd, af, lookup)));
+}
+
 SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
                              const int af, DNSLookup* lookup) :
     output_buffer_(new isc::util::OutputBuffer(0)),
     query_(new isc::dns::Message(isc::dns::Message::PARSE)),
     udp_endpoint_(sender_), lookup_callback_(lookup),
-    resume_called_(false), done_(false), stopped_(false),
-    recv_callback_(boost::bind(&SyncUDPServer::handleRead, this, _1, _2))
+    resume_called_(false), done_(false), stopped_(false)
 {
     if (af != AF_INET && af != AF_INET6) {
         isc_throw(InvalidParameter, "Address family must be either AF_INET "
@@ -71,8 +77,9 @@ SyncUDPServer::SyncUDPServer(asio::io_service& io_service, const int fd,
 
 void
 SyncUDPServer::scheduleRead() {
-    socket_->async_receive_from(asio::mutable_buffers_1(data_, MAX_LENGTH),
-                                sender_, recv_callback_);
+    socket_->async_receive_from(
+        asio::mutable_buffers_1(data_, MAX_LENGTH), sender_,
+        boost::bind(&SyncUDPServer::handleRead, shared_from_this(), _1, _2));
 }
 
 void

src/lib/asiodns/sync_udp_server.h

@@ -33,19 +33,26 @@
 #include <boost/function.hpp>
 #include <boost/noncopyable.hpp>
 #include <boost/scoped_ptr.hpp>
+#include <boost/enable_shared_from_this.hpp>
 
 #include <stdint.h>
 
 namespace isc {
 namespace asiodns {
 
+class SyncUDPServer;
+typedef boost::shared_ptr<SyncUDPServer> SyncUDPServerPtr;
+
 /// \brief An UDP server that doesn't asynchronous lookup handlers.
 ///
 /// That means, the lookup handler must provide the answer right away.
 /// This allows for implementation with less overhead, compared with
 /// the \c UDPServer class.
-class SyncUDPServer : public DNSServer, public boost::noncopyable {
-public:
+class SyncUDPServer : public DNSServer,
+                      public boost::enable_shared_from_this<SyncUDPServer>,
+                      boost::noncopyable
+{
+private:
     /// \brief Constructor
     ///
     /// Due to the nature of this server, it's meaningless if the lookup
@@ -70,6 +77,10 @@ public:
     SyncUDPServer(asio::io_service& io_service, const int fd, const int af,
                   DNSLookup* lookup);
 
+public:
+    static SyncUDPServerPtr create(asio::io_service& io_service, const int fd,
+                                   const int af, DNSLookup* lookup);
+
     /// \brief Start the SyncUDPServer.
     ///
     /// This is the function operator to keep interface with other server
@@ -156,24 +167,6 @@ private:
     // Placeholder for error code object.  It will be passed to ASIO library
     // to have it set in case of error.
     asio::error_code ec_;
-    // The callback functor for internal asynchronous read event.  This is
-    // stateless (and it will be copied in the ASIO library anyway), so
-    // can be const.
-    // SunStudio doesn't like a boost::function object to be passed, so
-    // we use the wrapper class as a workaround.
-    class CallbackWrapper {
-    public:
-        CallbackWrapper(boost::function<void(const asio::error_code&, size_t)>
-                        callback) :
-            callback_(callback)
-        {}
-        void operator()(const asio::error_code& error, size_t len) {
-            callback_(error, len);
-        }
-    private:
-        boost::function<void(const asio::error_code&, size_t)> callback_;
-    };
-    const CallbackWrapper recv_callback_;
 
     // Auxiliary functions
 

src/lib/asiodns/tests/dns_server_unittest.cc

@@ -527,8 +527,7 @@ boost::shared_ptr<SyncUDPServer>
 FdInit<SyncUDPServer>::createServer(int fd, int af) {
     delete this->lookup_;
     this->lookup_ = new SyncDummyLookup;
-    return (boost::shared_ptr<SyncUDPServer>(
-                new SyncUDPServer(this->service, fd, af, this->lookup_)));
+    return (SyncUDPServer::create(this->service, fd, af, this->lookup_));
 }
 
 // This makes it the template as gtest wants it.
@@ -763,7 +762,7 @@ TEST_F(SyncServerTest, mustResume) {
 
 // SyncUDPServer doesn't allow NULL lookup callback.
 TEST_F(SyncServerTest, nullLookupCallback) {
-    EXPECT_THROW(SyncUDPServer(service, 0, AF_INET, NULL),
+    EXPECT_THROW(SyncUDPServer::create(service, 0, AF_INET, NULL),
                  isc::InvalidParameter);
 }