From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Mon, 30 Jan 2012 17:13:52 +0000 (-0700)
Subject: Bug 3441: Part 1: Minimize cache size corruption by malformed swap.state.
X-Git-Tag: BumpSslServerFirst.take05~12^2~66
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3f9d4dc21bce9043c3ded0607a36a7883995b44d;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 fb1e50fb17..47b7468605 100644
--- 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 abbfb766d8..ced16f5971 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 063aeb8b22..7cebc1fa67 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 291eae91f3..80a366520b 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