From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 24 Oct 2013 15:27:49 +0000 (-0600)
Subject: Bug 3480: StoreEntry::kickProducer() segfaults in store_client::copy()
X-Git-Tag: SQUID_3_3_10~10
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0987957340db0b9a79a7781e363f395e741e2821;p=thirdparty%2Fsquid.git

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.
---

diff --git a/src/store_client.cc b/src/store_client.cc
index 1c74acf05a..f0c30b2cc8 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -264,12 +264,20 @@ store_client::copy(StoreEntry * anEntry,
     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 @@ storeClientCopy2(StoreEntry * e, store_client * sc)
     /* 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 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
 
     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 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
     e->kickProducer();
 #endif
 
+    e->unlock(); // after the "++e->lock_count" above
     return 1;
 }