------------------------------------------------------------
revno: 12639
revision-id: squid3@treenet.co.nz-20131024152749-36g5x19fqnel1bdj
parent: squid3@treenet.co.nz-20131024152621-xzfpnzkkzib4z9no
fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3480
author: Alex Rousskov <rousskov@measurement-factory.com>
committer: Amos Jeffries <squid3@treenet.co.nz>
branch nick: 3.3
timestamp: Thu 2013-10-24 09:27:49 -0600
message:
  Bug 3480: StoreEntry::kickProducer() segfaults in store_client::copy()
  context
  
  Short-term fix: Lock StoreEntry object so that it is not freed by
  storeClientCopy2() callbacks. Also lock StoreEntry in storeUnregister()
  context because an aborting entry may be deleted there unless it is
  double-locked.
  
  See bug 3480 comment #27 for detailed call stack analysis. Additional 
  cases
  include rejected copied HIT due to Var mismatch and hits blocked by
  reply_from_cache directive (under development; see bug 3937).
  
  Long-term, we need to make store copying asynchronous and revise 
  StoreEntry
  locking approach.
------------------------------------------------------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: squid3@treenet.co.nz-20131024152749-36g5x19fqnel1bdj
# target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# testament_sha1: 15321ed3f8182844ab62839e11f8edfc327a1b10
# timestamp: 2013-10-24 15:46:55 +0000
# source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# base_revision_id: squid3@treenet.co.nz-20131024152621-\
#   xzfpnzkkzib4z9no
# 
# Begin patch
=== modified file 'src/store_client.cc'
--- src/store_client.cc	2013-09-29 18:12:26 +0000
+++ src/store_client.cc	2013-10-24 15:27:49 +0000
@@ -264,12 +264,20 @@
     PROF_stop(storeClient_kickReads);
     copying = false;
 
+    // XXX: storeClientCopy2 calls doCopy() whose callback may free 'this'!
+    // We should make store copying asynchronous, to avoid worrying about
+    // 'this' being secretly deleted while we are still inside the object.
+    // For now, lock and use on-stack objects after storeClientCopy2().
+    ++anEntry->lock_count;
+
     storeClientCopy2(entry, this);
 
 #if USE_ADAPTATION
-    if (entry)
-        entry->kickProducer();
+    anEntry->kickProducer();
 #endif
+
+    anEntry->unlock(); // after the "++enEntry->lock_count" above
+    // Add no code here. This object may no longer exist.
 }
 
 /*
@@ -333,6 +341,9 @@
     /* Warning: doCopy may indirectly free itself in callbacks,
      * hence the lock to keep it active for the duration of
      * this function
+     * XXX: Locking does not prevent calling sc destructor (it only prevents
+     * freeing sc memory) so sc may become invalid from C++ p.o.v.
+     *
      */
     cbdataInternalLock(sc);
     assert (sc->flags.store_copying == 0);
@@ -727,7 +738,14 @@
 
     delete sc;
 
+    // This old assert seemed to imply that a locked entry cannot be deleted,
+    // but this entry may be deleted because StoreEntry::abort() unlocks it.
     assert(e->lock_count > 0);
+    // Since lock_count of 1 is not sufficient to prevent entry destruction,
+    // we must lock again so that we can dereference e after CheckQuickAbort().
+    // Do not call expensive StoreEntry::lock() here; e "use" has been counted.
+    // TODO: Separate entry locking from "use" counting to make locking cheap.
+    ++e->lock_count;
 
     if (mem->nclients == 0)
         CheckQuickAbort(e);
@@ -738,6 +756,7 @@
     e->kickProducer();
 #endif
 
+    e->unlock(); // after the "++e->lock_count" above
     return 1;
 }