Commit 856d9ca8 authored by Anthony Ryan's avatar Anthony Ryan Committed by Jari Sundell

Eliminate use-after-free in DHT Server (#120)

Currently when a connection triggers a failed_transaction it gets
deleted, which is the expected behavior. Unfortunately any packets
left in the DHT packet priority queue will now have been free'd
leading to a scenario where the process_queue is working on memory
which may contain unpredictable data.

The new behavior proposed by this patch is to also drop any queued
packets to prevent future processing since we're throwing out that
object for probelmatic behavior already.

Notably this seems to reflect the same behavior seen in issue #68
however I do not believe that to be a complete fix. It seems to
decrease the probability of the issue occurring. I personally
believe this fix should replace the one applied after #68 but am
certainly open to further discussion on the matter.

--------

READ of size 4 thread 0 (rtorrent main)

0. rak::socket_address_inet::address_n() const ../../rak/socket_address.h:167
1. torrent::DhtTransaction::key(rak::socket_address const*, int) (/usr/lib64/libtorrent.so.19+0x2d9936)
2. torrent::DhtTransaction::key(int) const (/usr/lib64/libtorrent.so.19+0x2d8d3f)
3. torrent::DhtServer::process_queue(std::deque<torrent::DhtTransactionPacket*, std::allocator<torrent::DhtTransactionPacket*> >&, unsigned int*) libtorrent/src/dht/dht_server.cc:801
4. torrent::DhtServer::event_write() libtorrent/src/dht/dht_server.cc:866
5. torrent::PollEPoll::perform() libtorrent/src/torrent/poll_epoll.cc:190
6. torrent::PollEPoll::do_poll(long, int) libtorrent/src/torrent/poll_epoll.cc:219
7. torrent::thread_base::event_loop(torrent::thread_base*) libtorrent/src/torrent/utils/thread_base.cc:174
8. main rtorrent/src/main.cc:867
9. __libc_start_main (/lib64/libc.so.6+0x20733)
10. _start (/usr/bin/rtorrent+0x9bce8)

freed by thread 0 (rtorrent main) here:

0. operator delete(void*) (/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/libasan.so.1+0x62e47)
1. torrent::DhtTransactionPing::~DhtTransactionPing() libtorrent/src/dht/dht_transaction.h:359
2. torrent::DhtServer::failed_transaction(std::_Rb_tree_iterator<std::pair<unsigned long const, torrent::DhtTransaction*> >, bool) libtorrent/src/dht/dht_server.cc:672
3. torrent::DhtServer::receive_timeout() libtorrent/src/dht/dht_server.cc:899
4. std::tr1::_Mem_fn<void (torrent::DhtServer::*)()>::operator()(torrent::DhtServer*) const /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/tr1/functional:585
5. std::tr1::result_of<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (std::tr1::result_of<std::tr1::_Mu<torrent::DhtServer*, false, false> (torrent::DhtServer*,
std::tr1::tuple<>)>::type)>::type std::tr1::_Bind<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (torrent::DhtServer*)>::__call<, 0>(std::tr1::tuple<> const&,
std::tr1::_Index_tuple<0>) /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/tr1/functional:1178
6. std::tr1::result_of<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (std::tr1::result_of<std::tr1::_Mu<torrent::DhtServer*, false, false> (torrent::DhtServer*,
std::tr1::tuple<>)>::type)>::type std::tr1::_Bind<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (torrent::DhtServer*)>::operator()<>()
(/usr/lib64/libtorrent.so.19+0x2e1e5d)
7. std::tr1::_Function_handler<void (), std::tr1::_Bind<std::tr1::_Mem_fn<void (torrent::DhtServer::*)()> (torrent::DhtServer*)> >::_M_invoke(std::tr1::_Any_data const&) /usr/lib/gcc/x86_64-pc-linux-gnu/4.9.3/include/g++-v4/tr1/functional:1796
8. std::tr1::function<void ()>::operator()() const (/usr/bin/rtorrent+0xa7b7b)
9. torrent::thread_main::call_events() libtorrent/src/thread_main.cc:82
10. torrent::thread_base::event_loop(torrent::thread_base*) libtorrent/src/torrent/utils/thread_base.cc:141
11. main rtorrent/src/main.cc:867
12. __libc_start_main (/lib64/libc.so.6+0x20733)
13. _start (/usr/bin/rtorrent+0x9bce8)
parent 7a59675c
......@@ -205,7 +205,7 @@ DhtServer::stop() {
}
void
DhtServer::reset_statistics() {
DhtServer::reset_statistics() {
m_queriesReceived = 0;
m_queriesSent = 0;
m_repliesReceived = 0;
......@@ -531,6 +531,12 @@ DhtServer::add_packet(DhtTransactionPacket* packet, int priority) {
}
}
void
DhtServer::drop_packet(DhtTransactionPacket* packet) {
m_highQueue.erase(std::remove(m_highQueue.begin(), m_highQueue.end(), packet), m_highQueue.end());
m_lowQueue.erase(std::remove(m_lowQueue.begin(), m_lowQueue.end(), packet), m_lowQueue.end());
}
void
DhtServer::create_query(transaction_itr itr, int tID, const rak::socket_address* sa, int priority) {
if (itr->second->id() == m_router->id())
......@@ -670,6 +676,7 @@ DhtServer::failed_transaction(transaction_itr itr, bool quick) {
} catch (std::exception& e) {
if (!quick) {
drop_packet(transaction->packet());
delete itr->second;
m_transactions.erase(itr);
}
......@@ -682,6 +689,7 @@ DhtServer::failed_transaction(transaction_itr itr, bool quick) {
return ++itr; // don't actually delete the transaction until the final timeout
} else {
drop_packet(transaction->packet());
delete itr->second;
m_transactions.erase(itr++);
return itr;
......
......@@ -149,6 +149,7 @@ private:
void find_node_next(DhtTransactionSearch* t);
void add_packet(DhtTransactionPacket* packet, int priority);
void drop_packet(DhtTransactionPacket* packet);
void create_query(transaction_itr itr, int tID, const rak::socket_address* sa, int priority);
void create_response(const DhtMessage& req, const rak::socket_address* sa, DhtMessage& reply);
void create_error(const DhtMessage& req, const rak::socket_address* sa, int num, const char* msg);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment