Random segfaults due to writes to free'd memory

[dishather]

In DhtServer::process_queue, packet's transaction may get deleted if hard timeout is hit. But then, the deleted transaction is modified by set_packet(NULL). This leads to random segfaults in rtorrent, especially when there are lots of open downloads.

Here is a fix:

diff --git a/src/dht/dht_server.cc b/src/dht/dht_server.cc
index 844d5f7..a7a6248 100644
--- a/src/dht/dht_server.cc
+++ b/src/dht/dht_server.cc
@@ -796,6 +796,9 @@ DhtServer::process_queue(packet_queue& queue, uint32_t* quota) {

   while (!queue.empty()) {
     DhtTransactionPacket* packet = queue.front();
+    DhtTransaction::key_type transactionKey = 0;
+    if(packet->has_transaction())
+      transactionKey = packet->transaction()->key(packet->id());

     // Make sure its transaction hasn't timed out yet, if it has/had one
     // and don't bother sending non-transaction packets (replies) after
@@ -828,7 +831,7 @@ DhtServer::process_queue(packet_queue& queue, uint32_t* quota) {
     } catch (network_error& e) {
       // Couldn't write packet, maybe something wrong with node address or routing, so mark node as bad.
       if (packet->has_transaction()) {
-        transaction_itr itr = m_transactions.find(packet->transaction()->key(packet->id()));
+        transaction_itr itr = m_transactions.find(transactionKey);
         if (itr == m_transactions.end())
           throw internal_error("DhtServer::process_queue could not find transaction.");

@@ -836,8 +839,12 @@ DhtServer::process_queue(packet_queue& queue, uint32_t* quota) {
       }
     }

-    if (packet->has_transaction())
-      packet->transaction()->set_packet(NULL);
+    if (packet->has_transaction()) {
+      // here transaction can be already deleted by failed_transaction.
+      transaction_itr itr = m_transactions.find(transactionKey);
+      if (itr != m_transactions.end())
+        packet->transaction()->set_packet(NULL);
+    }

     delete packet;
   }

[chros73]

This can be closed since it's already in master branch: fd8191e

[chros73]

Btw, is there any particular reason for releases and master branch being different at the time of a release?
It's really confusing in this way.
For example, I just found out by accident when I wanted to create a patch for this that it's already in master branch but not in 0.9.6.

[rakshasa]

You should look at "branch-0.9" or "branch-0.13" for that.

Master is what I use and merge into, not the usual stable branches.

[chros73]

Master is what I use and merge into, not the usual stable branches.

Oh, I see. So the master branch is the develop branch :)

I still think that it would be meaningful if master branch would be the same as a stable branch at the time of a release, especially in the long run, since now (hopefully) more and more people will create changes to master that will be problematic to merge to a different stable branch.
It just a thought, though :)

[chros73]

I still think that it would be meaningful if master branch would be the same as a stable branch at the time of a release, especially in the long run...

I'm using git version of both projects (ce00085 (2016-09-09), c97b462 (2016-09-05)) (including my pending patches as well) for about a month now without any new issues that weren't in 0.9.6 version:
if there isn't any other reason to follow the cherry-picking approach for the next release (e.g. compiler dependencies, etc.), I suggest to make new ones from the master branches (there are 4 years old codes there that weren't widely tested).

And can you look at this rTorrent reading 4x more data than sending issue as well before the next release, it's 99% that it affect everybody.
Cheers