From bd3e2a5ac7da4f31a21c2a42c55d221fadd179e8 Mon Sep 17 00:00:00 2001
From: Anthony Ryan <anthonyryan1@gmail.com>
Date: Sun, 13 Nov 2016 08:41:51 -0500
Subject: [PATCH] Drop packets everywhere we're deleting DhtTransaction's
 (#135)

This is a continuation of 856d9ca810d437656c526d40cdded14d095e6695

I've observed at least two more behaviours where this deletion of
the DhtTransaction within dht_server.cc can later lead to a use
after free when attempting to process the associated packets.

As such this commit updates all of the areas where we're deleting
DhtTransactions to also drop the corresponding packets. While this
arguably could be placed within the DhtTransactions' destructor, it
feels like that would be messing up the hierarchy within the Dht*
classes.
---
 src/dht/dht_server.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/dht/dht_server.cc b/src/dht/dht_server.cc
index f8e84edeb..3bd3654c0 100644
--- a/src/dht/dht_server.cc
+++ b/src/dht/dht_server.cc
@@ -263,6 +263,7 @@ DhtServer::cancel_announce(DownloadInfo* info, const TrackerDht* tracker) {
       DhtAnnounce* announce = static_cast<DhtAnnounce*>(itr->second->as_search()->search());
 
       if ((info == NULL || announce->target() == info->hash()) && (tracker == NULL || announce->tracker() == tracker)) {
+        drop_packet(itr->second->packet());
         delete itr->second;
         m_transactions.erase(itr++);
         continue;
@@ -409,6 +410,7 @@ DhtServer::process_response(const HashString& id, const rak::socket_address* sa,
     m_router->node_replied(id, sa);
 
   } catch (std::exception& e) {
+    drop_packet(itr->second->packet());
     delete itr->second;
     m_transactions.erase(itr);
 
@@ -416,6 +418,7 @@ DhtServer::process_response(const HashString& id, const rak::socket_address* sa,
     throw;
   }
 
+  drop_packet(itr->second->packet());
   delete itr->second;
   m_transactions.erase(itr);
 }
@@ -436,6 +439,7 @@ DhtServer::process_error(const rak::socket_address* sa, const DhtMessage& error)
   // If it consistently returns errors for valid queries it's probably broken.  But a
   // few error messages are acceptable. So we do nothing and pretend the query never happened.
 
+  drop_packet(itr->second->packet());
   delete itr->second;
   m_transactions.erase(itr);
 }
@@ -698,8 +702,10 @@ DhtServer::failed_transaction(transaction_itr itr, bool quick) {
 
 void
 DhtServer::clear_transactions() {
-  for (transaction_map::iterator itr = m_transactions.begin(), last = m_transactions.end(); itr != last; itr++)
+  for (transaction_map::iterator itr = m_transactions.begin(), last = m_transactions.end(); itr != last; itr++) {
+    drop_packet(itr->second->packet());
     delete itr->second;
+  }
 
   m_transactions.clear();
 }