From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sat, 4 Feb 2012 05:46:26 +0000 (-0700)
Subject: Bug 3441: Part 1: Minimize cache size corruption by malformed swap.state.
X-Git-Tag: SQUID_3_1_19~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a941c13d6dc997149ecd4f25aef38ddcbe4605d4;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 1892cb53d3..11133b9009 100644
--- a/src/StoreSwapLogData.cc
+++ b/src/StoreSwapLogData.cc
@@ -39,6 +39,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 782dc6f2a7..4e919bd10b 100644
--- a/src/cache_diff.cc
+++ b/src/cache_diff.cc
@@ -180,7 +180,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 6da3051533..c016e9c93a 100644
--- a/src/fs/ufs/ufscommon.cc
+++ b/src/fs/ufs/ufscommon.cc
@@ -604,11 +604,10 @@ RebuildState::rebuildFromSwapLog()
 
         n_read++;
 
-        if (swapData.op <= SWAP_LOG_NOP)
-            continue;
-
-        if (swapData.op >= SWAP_LOG_MAX)
+        if (!swapData.sane()) {
+            counts.invalid++;
             continue;
+        }
 
         /*
          * BC: during 2.4 development, we changed the way swap file