Detail swapfile header inconsistencies.

Squid may fail to load cache entry metadata for several very different
reasons, including the following two relatively common ones:

* A cache_dir entry corruption.
* Huge cache_dir entry metadata that does not fit into the I/O buffer
  used for loading entry metadata.

Knowing the exact failure reason may help triage and guide development.
We refactored existing checks to distinguish various error cases,
including the two above. Refactoring also reduced code duplication.

These improvements also uncovered and fixed a null pointer dereference
inside ufsdump.cc (but ufsdump does not even build right now for reasons
unrelated to these changes).

diff --git a/src/StoreMetaUnpacker.cc b/src/StoreMetaUnpacker.cc
index 5b5cfb737de30a6cd89c7b8a40b2c2ee470d5a64..5bb86796be1a68e3421492221685c0de6cb5a823 100644 (file)
--- a/src/StoreMetaUnpacker.cc
+++ b/src/StoreMetaUnpacker.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 20    Storage Manager Swapfile Unpacker */
 
 #include "squid.h"
+#include "base/TextException.h"
 #include "Debug.h"
 #include "defines.h"
 #include "StoreMeta.h"
@@ -34,25 +35,21 @@ StoreMetaUnpacker::isBufferZero()
     return true;
 }
 
-bool
-StoreMetaUnpacker::isBufferSane()
+void
+StoreMetaUnpacker::checkBuffer()
 {
-    if (buf[0] != (char) STORE_META_OK)
-        return false;
-
+    assert(buf); // paranoid; already checked in the constructor
+    if (buf[0] != static_cast<char>(STORE_META_OK))
+        throw TexcHere("store entry metadata is corrupted");
     /*
      * sanity check on 'buflen' value.  It should be at least big
      * enough to hold one type and one length.
      */
     getBufferLength();
-
     if (*hdr_len < MinimumBufferLength)
-        return false;
-
+        throw TexcHere("store entry metadata is too small");
     if (*hdr_len > buflen)
-        return false;
-
-    return true;
+        throw TexcHere("store entry metadata is too big");
 }
 
 void
@@ -122,8 +119,7 @@ StoreMetaUnpacker::createStoreMeta ()
     tail = &TLV;
     assert(hdr_len != NULL);
 
-    if (!isBufferSane())
-        return NULL;
+    checkBuffer();
 
     getBufferLength();
 
@@ -134,6 +130,10 @@ StoreMetaUnpacker::createStoreMeta ()
             break;
     }
 
+    if (!TLV)
+       throw TexcHere("store entry metadata is empty");
+
+    assert(TLV);
     return TLV;
 }
 

diff --git a/src/StoreMetaUnpacker.h b/src/StoreMetaUnpacker.h
index 23f8f8a1d030775ec572fab0934ce99de4fd68b0..a7a2be88180695bfcae6420679d9397cabfd511b 100644 (file)
--- a/src/StoreMetaUnpacker.h
+++ b/src/StoreMetaUnpacker.h
@@ -18,8 +18,9 @@ class StoreMetaUnpacker
 public:
     StoreMetaUnpacker (const char *buf, ssize_t bufferLength, int *hdrlen);
     StoreMeta *createStoreMeta();
-    bool isBufferZero(); ///< all-zeros buffer, implies !isBufferSane
-    bool isBufferSane();
+    bool isBufferZero(); ///< all-zeros buffer, checkBuffer() would throw
+    /// validates buffer sanity and throws if validation fails
+    void checkBuffer();
 
 private:
     static int const MinimumBufferLength;

diff --git a/src/fs/rock/RockHeaderUpdater.cc b/src/fs/rock/RockHeaderUpdater.cc
index 833ac5bb35b6ee69dd80574049a09fac9008e24f..981ae8281cfd2627a4db6aca56b135633aa205fe 100644 (file)
--- a/src/fs/rock/RockHeaderUpdater.cc
+++ b/src/fs/rock/RockHeaderUpdater.cc
@@ -244,7 +244,7 @@ Rock::HeaderUpdater::parseReadBytes()
             exchangeBuffer.length(),
             &staleSwapHeaderSize);
         // Squid assumes that metadata always fits into a single db slot
-        Must(aBuilder.isBufferSane()); // cannot update what we cannot parse
+        aBuilder.checkBuffer(); // cannot update an entry with invalid metadata
         debugs(47, 7, "staleSwapHeaderSize=" << staleSwapHeaderSize);
         Must(staleSwapHeaderSize > 0);
         exchangeBuffer.consume(staleSwapHeaderSize);

diff --git a/src/store_client.cc b/src/store_client.cc
index 1d4b67eeca5a7311001616a2ae6e25610c920834..05423715177b45baa97e93f71efbb42530f5b855 100644 (file)
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -533,20 +533,15 @@ store_client::unpackHeader(char const *buf, ssize_t len)
     }
 
     int swap_hdr_sz = 0;
-    StoreMetaUnpacker aBuilder(buf, len, &swap_hdr_sz);
-
-    if (!aBuilder.isBufferSane()) {
-        /* oops, bad disk file? */
-        debugs(90, DBG_IMPORTANT, "WARNING: swapfile header inconsistent with available data");
-        return false;
-    }
-
-    tlv *tlv_list = aBuilder.createStoreMeta ();
-
-    if (tlv_list == NULL) {
-        debugs(90, DBG_IMPORTANT, "WARNING: failed to unpack meta data");
+    tlv *tlv_list = nullptr;
+    try {
+        StoreMetaUnpacker aBuilder(buf, len, &swap_hdr_sz);
+        tlv_list = aBuilder.createStoreMeta();
+    } catch (const std::exception &e) {
+        debugs(90, DBG_IMPORTANT, "WARNING: failed to unpack metadata because " << e.what());
         return false;
     }
+    assert(tlv_list);
 
     /*
      * Check the meta data and make sure we got the right object.

diff --git a/src/store_rebuild.cc b/src/store_rebuild.cc
index 178c054eb447b6523caee596c4f03e5f5e2ddcb6..635b4adbcf2b1482f2a2e2ff053e27f38ac1d092 100644 (file)
--- a/src/store_rebuild.cc
+++ b/src/store_rebuild.cc
@@ -309,17 +309,14 @@ storeRebuildParseEntry(MemBuf &buf, StoreEntry &tmpe, cache_key *key,
         return false;
     }
 
-    if (!aBuilder.isBufferSane()) {
-        debugs(47, DBG_IMPORTANT, "WARNING: Ignoring malformed cache entry.");
-        return false;
-    }
-
-    StoreMeta *tlv_list = aBuilder.createStoreMeta();
-    if (!tlv_list) {
-        debugs(47, DBG_IMPORTANT, "WARNING: Ignoring cache entry with invalid " <<
-               "meta data");
+    StoreMeta *tlv_list = nullptr;
+    try {
+        tlv_list = aBuilder.createStoreMeta();
+    } catch (const std::exception &e) {
+        debugs(47, DBG_IMPORTANT, "WARNING: Ignoring store entry because " << e.what());
         return false;
     }
+    assert(tlv_list);
 
     // TODO: consume parsed metadata?
 

diff --git a/src/ufsdump.cc b/src/ufsdump.cc
index 9bbb9d72e622485e8802aa5d898c71218f06b1b8..fa895ef733bc6f229c4ce805b8a86659745897d7 100644 (file)
--- a/src/ufsdump.cc
+++ b/src/ufsdump.cc
@@ -162,8 +162,8 @@ main(int argc, char *argv[])
         for_each(*metadata, dumper);
 
         return 0;
-    } catch (std::runtime_error error) {
-        std::cout << "Failed : " << error.what() << std::endl;
+    } catch (const std::exception &e) {
+        std::cout << "Failed : " << e.what() << std::endl;
 
         if (fd >= 0)
             close(fd);