From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 2 Feb 2012 08:48:59 +0000 (-0700)
Subject: Bug 3441: Part 1: Minimize cache size corruption by malformed swap.state.
X-Git-Tag: SQUID_3_2_0_15~7
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=17143017f0cfeb0be458d265ffd5e481cd9f7aec;p=thirdparty%2Fsquid.git

Bug 3441: Part 1: Minimize cache size corruption by malformed swap.state.

If swap.state gets corrupted, a single entry with bogus size value will screw
up Squid idea of the current cache size. A newly added StoreSwapLogData sane()
method attempts to minimize the chance of corruption by ignoring log entries
with obviously bogus values.

However, without expensive size checks (-S or "Does the log entry matches the
actual cache file size?"), it is not possible to reliably detect all bogus log
entries.

If Squid gets a wrong idea of the current cache size, it may either cache too
much (and possibly run out of space) OR delete everything.
---

diff --git a/src/StoreSwapLogData.cc b/src/StoreSwapLogData.cc
index 24ae521120..ba0dec39ea 100644
--- a/src/StoreSwapLogData.cc
+++ b/src/StoreSwapLogData.cc
@@ -41,6 +41,24 @@ StoreSwapLogData::StoreSwapLogData(): op(0), swap_filen (0), timestamp (0), last
     memset (key, '\0', sizeof(key));
 }
 
+bool
+StoreSwapLogData::sane() const
+{
+    // TODO: These checks are rather weak. A corrupted swap.state may still
+    // cause havoc (e.g., cur_size may become astronomical). Add checksums?
+
+    const time_t minTime = -2; // -1 is common; expires sometimes uses -2
+
+    // Check what we safely can; for some fields any value might be valid
+    return SWAP_LOG_NOP < op && op < SWAP_LOG_MAX &&
+           swap_filen >= 0 &&
+           timestamp >= minTime &&
+           lastref >= minTime &&
+           expires >= minTime &&
+           lastmod >= minTime &&
+           swap_file_sz > 0; // because swap headers ought to consume space
+}
+
 StoreSwapLogHeader::StoreSwapLogHeader():op(SWAP_LOG_VERSION), version(1)
 {
     record_size = sizeof(StoreSwapLogData);
diff --git a/src/StoreSwapLogData.h b/src/StoreSwapLogData.h
index 5fad46944f..0ea668ab90 100644
--- a/src/StoreSwapLogData.h
+++ b/src/StoreSwapLogData.h
@@ -86,6 +86,9 @@ public:
     MEMPROXY_CLASS(StoreSwapLogData);
     StoreSwapLogData();
 
+    /// consistency self-check: whether the data appears to make sense
+    bool sane() const;
+
     /**
      * Either SWAP_LOG_ADD when an object is added to the disk storage,
      * or SWAP_LOG_DEL when an object is deleted.
diff --git a/src/cache_diff.cc b/src/cache_diff.cc
index 3c50a27db2..5197a65b8c 100644
--- a/src/cache_diff.cc
+++ b/src/cache_diff.cc
@@ -179,7 +179,7 @@ cacheIndexScan(CacheIndex * idx, const char *fname, FILE * file)
     while (fread(&s, sizeof(s), 1, file) == 1) {
         count++;
         idx->scanned_count++;
-        /* if (s.op <= SWAP_LOG_NOP || s.op >= SWAP_LOG_MAX)
+        /* if (!s.sane())
          * continue; */
 
         if (s.op == SWAP_LOG_ADD) {
diff --git a/src/fs/ufs/ufscommon.cc b/src/fs/ufs/ufscommon.cc
index a24e7251e9..3ee6b7c7fd 100644
--- a/src/fs/ufs/ufscommon.cc
+++ b/src/fs/ufs/ufscommon.cc
@@ -491,11 +491,10 @@ RebuildState::rebuildFromSwapLog()
 
     n_read++;
 
-    if (swapData.op <= SWAP_LOG_NOP)
-        return;
-
-    if (swapData.op >= SWAP_LOG_MAX)
+    if (!swapData.sane()) {
+        counts.invalid++;
         return;
+    }
 
     /*
      * BC: during 2.4 development, we changed the way swap file