------------------------------------------------------------
revno: 13011
revision-id: squid3@treenet.co.nz-20131023051017-wkiria1vr429hcgg
parent: squid3@treenet.co.nz-20131013134155-khinz1g5an15opg7
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.4
timestamp: Tue 2013-10-22 23:10:17 -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-20131023051017-wkiria1vr429hcgg
# target_branch: http://bzr.squid-cache.org/bzr/squid3/3.4
# testament_sha1: 85c8f207781498e368f96d5a14a4c8321dc30c8b
# timestamp: 2013-10-23 05:55:33 +0000
# source_branch: http://bzr.squid-cache.org/bzr/squid3/3.4
# base_revision_id: squid3@treenet.co.nz-20131013134155-\
#   khinz1g5an15opg7
# 
# Begin patch
=== modified file 'src/store_client.cc'
--- src/store_client.cc	2013-09-29 13:45:05 +0000
+++ src/store_client.cc	2013-10-23 05:10:17 +0000
@@ -249,12 +249,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.
 }
 
 /*
@@ -318,6 +326,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);
@@ -712,7 +723,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);
@@ -723,6 +741,7 @@
     e->kickProducer();
 #endif
 
+    e->unlock(); // after the "++e->lock_count" above
     return 1;
 }