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 fb1e50fb172a8524421884ac2c5c714ddfd48595..47b7468605114660248d04de9163709dfec02bad 100644 (file)
--- a/src/StoreSwapLogData.cc
+++ b/src/StoreSwapLogData.cc
@@ -41,6 +41,22 @@ 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?
+
+    // 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 >= -1 && // in case some code is using -1 to mean "n/a"
+        lastref >= -1 &&
+        expires >= -1 &&
+        lastmod >= -1 &&
+        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 abbfb766d803deaa20f8806baebb15e0b5c21114..ced16f59717faa2a6f342ed5577bd6a30dda8e25 100644 (file)
--- 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 063aeb8b22b79c301344a3e439ca8c0ac88d1251..7cebc1fa67b8f692d8914a6017d4b8eff043cb6f 100644 (file)
--- 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 291eae91f3dda2f048bd69bdef00953b62e4a003..80a366520b1c574185221851b7a718ef40d8a4db 100644 (file)
--- 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