Avoid "FATAL: Squid has attempted to read data from memory that is not present"

crashes. Improve related code.

Shared memory caching code was not checking whether the response is generally
cachable and, hence, tried to store responses that Squid already refused to
cache (e.g., release-requested entries). The shared memory cache should also
check that the response is contiguous because the disk store may not check
that (e.g., when disk caching id disabled).

The problem was exacerbated by the broken entry dump code accompanying the
FATAL message. The Splay tree iterator is evidently not iterating a portion of
the tree. I added a visitor pattern code to work around that, but did not try
to fix the Splay iterator iterator itself because, in part, the whole iterator
design felt inappropriate (storing and flattening the tree before iterating
it?) for its intended purpose. I could not check quickly enough where the
broken iterator is used besides mem_hdr::debugDump() so more bugs like this
are possible.

Optimized StoreEntry::checkCachable() a little and removed wrong TODO comment:
Disk-only mayStartSwapOut() should not accumulate "general" cachability checks
which are used by RAM caches as well.

Added more mem_hdr debugging and polished method descriptions.

Fixed NullStoreEntry::mayStartSwapout() spelling/case. It should override
StoreEntry::mayStartSwapOut().

diff --git a/include/splay.h b/include/splay.h
index ea4f2c3e036ac460f814aaccee6d6da84e04c9c5..d4e4202fe87fb7ce088d920afa1fa6710d2cdbb1 100644 (file)
--- a/include/splay.h
+++ b/include/splay.h
@@ -31,6 +31,9 @@ public:
     SplayNode<V> * insert(Value data, SPLAYCMP * compare);
 
     template <class FindValue> SplayNode<V> * splay(const FindValue &data, int( * compare)(FindValue const &a, Value const &b)) const;
+
+    /// recursively visit left nodes, this node, and then right nodes
+    template <class Visitor> void visit(Visitor &v) const;
 };
 
 typedef SplayNode<void *> splayNode;
@@ -71,6 +74,9 @@ public:
 
     const_iterator end() const;
 
+    /// recursively visit all nodes, in left-to-right order
+    template <class Visitor> void visit(Visitor &v) const;
+
     size_t elements;
 };
 
@@ -274,6 +280,25 @@ SplayNode<V>::splay(FindValue const &dataToFind, int( * compare)(FindValue const
     return top;
 }
 
+template <class V>
+template <class Visitor>
+void
+SplayNode<V>::visit(Visitor &visitor) const {
+    if (left)
+        left->visit(visitor);
+    visitor(data);
+    if (right)
+        right->visit(visitor);
+}
+
+template <class V>
+template <class Visitor>
+void
+Splay<V>::visit(Visitor &visitor) const {
+    if (head)
+        head->visit(visitor);
+}
+
 template <class V>
 template <class FindValue>
 typename Splay<V>::Value const *
@@ -360,6 +385,7 @@ Splay<V>::end() const
     return const_iterator(NULL);
 }
 
+// XXX: This does not seem to iterate the whole thing in some cases.
 template <class V>
 class SplayConstIterator
 {

diff --git a/src/MemStore.cc b/src/MemStore.cc
index 29c1917418a7ebb85afdd72da70da3387b276054..f327bf1e5041a0eb88cbdc3f93cfbf73a6d51118 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -411,7 +411,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof)
 
 /// whether we should cache the entry
 bool
-MemStore::shouldCache(const StoreEntry &e) const
+MemStore::shouldCache(StoreEntry &e) const
 {
     if (e.mem_status == IN_MEMORY) {
         debugs(20, 5, "already loaded from mem-cache: " << e);
@@ -453,6 +453,11 @@ MemStore::shouldCache(const StoreEntry &e) const
         return false; // will not cache due to cachable entry size limits
     }
 
+    if (!e.mem_obj->isContiguous()) {
+        debugs(20, 5, "not contiguous");
+        return false;
+    }
+
     if (!map) {
         debugs(20, 5, HERE << "No map to mem-cache " << e);
         return false;

diff --git a/src/MemStore.h b/src/MemStore.h
index c536620863bc45ae0005873abbb152e3c86e6795..e67cc4a50655977d022615fad70457187cd6fb7e 100644 (file)
--- a/src/MemStore.h
+++ b/src/MemStore.h
@@ -58,7 +58,7 @@ public:
     static int64_t EntryLimit();
 
 protected:
-    bool shouldCache(const StoreEntry &e) const;
+    bool shouldCache(StoreEntry &e) const;
     bool startCaching(StoreEntry &e);
 
     void copyToShm(StoreEntry &e);

diff --git a/src/Store.h b/src/Store.h
index 241667358b653a709a9759e3682bebc17e2c0348..6a4c191c2bf287b9b1d58399c5548b3d82df752c 100644 (file)
--- a/src/Store.h
+++ b/src/Store.h
@@ -120,11 +120,14 @@ public:
     bool swappingOut() const { return swap_status == SWAPOUT_WRITING; }
     void swapOutFileClose(int how);
     const char *url() const;
+    /// generally cachable (checks agnostic to disk/RAM-specific requirements)
+    /// common part of mayStartSwapOut() and memoryCachable()
+    /// TODO: Make private so only those two methods can call this.
     int checkCachable();
     int checkNegativeHit() const;
     int locked() const;
     int validToSend() const;
-    bool memoryCachable() const; ///< may be cached in memory
+    bool memoryCachable(); ///< checkCachable() and can be cached in memory
 
     /// if needed, initialize mem_obj member w/o URI-related information
     MemObject *makeMemObject();
@@ -272,7 +275,7 @@ private:
     store_client_t storeClientType() const {return STORE_MEM_CLIENT;}
 
     char const *getSerialisedMetaData();
-    bool mayStartSwapout() {return false;}
+    virtual bool mayStartSwapOut() { return false; }
 
     void trimMemory(const bool preserveSwappable) {}
 

diff --git a/src/SwapDir.h b/src/SwapDir.h
index bd1e542c059b8bcdab7f35e5546110a6d0017d34..1dd99812e7eaa17a62483bca4edef22a761b1cf3 100644 (file)
--- a/src/SwapDir.h
+++ b/src/SwapDir.h
@@ -104,7 +104,7 @@ public:
 private:
     void createOneStore(Store &aStore);
     StoreEntry *find(const cache_key *key);
-    bool keepForLocalMemoryCache(const StoreEntry &e) const;
+    bool keepForLocalMemoryCache(StoreEntry &e) const;
     bool anchorCollapsed(StoreEntry &collapsed, bool &inSync);
     bool anchorCollapsedOnDisk(StoreEntry &collapsed, bool &inSync);
 

diff --git a/src/stmem.cc b/src/stmem.cc
index 011dca04630f2fa848c2e2557977ab4b987e1502..755a3634207e34390f2a6f190c6c45f58fc6722f 100644 (file)
--- a/src/stmem.cc
+++ b/src/stmem.cc
@@ -95,6 +95,7 @@ mem_hdr::unlink(mem_node *aNode)
         return false;
     }
 
+    debugs(19, 8, this << " removing " << aNode);
     nodes.remove (aNode, NodeCompare);
     delete aNode;
     return true;
@@ -103,6 +104,7 @@ mem_hdr::unlink(mem_node *aNode)
 int64_t
 mem_hdr::freeDataUpto(int64_t target_offset)
 {
+    debugs(19, 8, this << " up to " << target_offset);
     /* keep the last one to avoid change to other part of code */
     SplayNode<mem_node*> const * theStart;
 
@@ -232,7 +234,7 @@ mem_hdr::debugDump() const
     debugs (19, 0, "mem_hdr::debugDump: lowest offset: " << lowestOffset() << " highest offset + 1: " << endOffset() << ".");
     std::ostringstream result;
     PointerPrinter<mem_node *> foo(result, " - ");
-    for_each (getNodes().begin(), getNodes().end(), foo);
+    getNodes().visit(foo);
     debugs (19, 0, "mem_hdr::debugDump: Current available data is: " << result.str() << ".");
 }
 

diff --git a/src/store.cc b/src/store.cc
index 066cb456a25493856dd2007d82a9b0fa5ae32f9e..916aad2b9030b1fba093db3e2b2fab1ec85951a6 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -949,11 +949,23 @@ StoreEntry::checkTooSmall()
     return 0;
 }
 
-// TODO: remove checks already performed by swapoutPossible()
 // TODO: move "too many open..." checks outside -- we are called too early/late
 int
 StoreEntry::checkCachable()
 {
+    // XXX: This method is used for both memory and disk caches, but some
+    // checks are specific to disk caches. Move them to mayStartSwapOut().
+
+    // XXX: This method may be called several times, sometimes with different
+    // outcomes, making store_check_cachable_hist counters misleading.
+
+    // check this first to optimize handling of repeated calls for uncachables
+    if (EBIT_TEST(flags, RELEASE_REQUEST)) {
+        debugs(20, 2, "StoreEntry::checkCachable: NO: not cachable");
+        ++store_check_cachable_hist.no.not_entry_cachable; // TODO: rename?
+        return 0; // avoid rerequesting release below
+    }
+
 #if CACHE_ALL_METHODS
 
     if (mem_obj->method != Http::METHOD_GET) {
@@ -964,9 +976,6 @@ StoreEntry::checkCachable()
         if (store_status == STORE_OK && EBIT_TEST(flags, ENTRY_BAD_LENGTH)) {
             debugs(20, 2, "StoreEntry::checkCachable: NO: wrong content-length");
             ++store_check_cachable_hist.no.wrong_content_length;
-        } else if (EBIT_TEST(flags, RELEASE_REQUEST)) {
-            debugs(20, 2, "StoreEntry::checkCachable: NO: not cachable");
-            ++store_check_cachable_hist.no.not_entry_cachable; // TODO: rename?
         } else if (EBIT_TEST(flags, ENTRY_NEGCACHED)) {
             debugs(20, 3, "StoreEntry::checkCachable: NO: negative cached");
             ++store_check_cachable_hist.no.negative_cached;
@@ -1400,8 +1409,11 @@ storeConfigure(void)
 }
 
 bool
-StoreEntry::memoryCachable() const
+StoreEntry::memoryCachable()
 {
+    if (!checkCachable())
+        return 0;
+
     if (mem_obj == NULL)
         return 0;
 

diff --git a/src/store_dir.cc b/src/store_dir.cc
index 82d6d8324cb77e444a8446da73cc32d63920e1ec..8d124736b88af5bacc18478722dfa7d5dadc4cf5 100644 (file)
--- a/src/store_dir.cc
+++ b/src/store_dir.cc
@@ -865,7 +865,7 @@ void StoreController::markForUnlink(StoreEntry &e)
 // move this into [non-shared] memory cache class when we have one
 /// whether e should be kept in local RAM for possible future caching
 bool
-StoreController::keepForLocalMemoryCache(const StoreEntry &e) const
+StoreController::keepForLocalMemoryCache(StoreEntry &e) const
 {
     if (!e.memoryCachable())
         return false;

diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc
index 72b94c94afdd0c0991e12a2ab2f790d9a401397a..fd2bb4819a7972c8320242f941fe4784ee021ac1 100644 (file)
--- a/src/tests/stub_store.cc
+++ b/src/tests/stub_store.cc
@@ -50,7 +50,7 @@ int StoreEntry::checkCachable() STUB_RETVAL(0)
 int StoreEntry::checkNegativeHit() const STUB_RETVAL(0)
 int StoreEntry::locked() const STUB_RETVAL(0)
 int StoreEntry::validToSend() const STUB_RETVAL(0)
-bool StoreEntry::memoryCachable() const STUB_RETVAL(false)
+bool StoreEntry::memoryCachable() STUB_RETVAL(false)
 MemObject *StoreEntry::makeMemObject() STUB_RETVAL(NULL)
 void StoreEntry::createMemObject(const char *, const char *, const HttpRequestMethod &aMethod) STUB
 void StoreEntry::dump(int debug_lvl) const STUB