Skip to content

Commit

Permalink
Drop packets everywhere we're deleting DhtTransaction's (#135)
Browse files Browse the repository at this point in the history
This is a continuation of 856d9ca

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.
  • Loading branch information
anthonyryan1 authored and rakshasa committed Nov 13, 2016
1 parent 8d64035 commit bd3e2a5
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion src/dht/dht_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -409,13 +410,15 @@ 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);

m_errorsCaught++;
throw;
}

drop_packet(itr->second->packet());
delete itr->second;
m_transactions.erase(itr);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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();
}
Expand Down

0 comments on commit bd3e2a5

Please sign in to comment.