From 4c2f8b720a6c580ebaf37844ef3fb95ecac8e6eb Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Fri, 17 Mar 2017 20:44:27 +1300 Subject: [PATCH] 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). --- src/StoreMetaUnpacker.cc | 26 +++++++++++++------------- src/StoreMetaUnpacker.h | 5 +++-- src/fs/rock/RockHeaderUpdater.cc | 2 +- src/store_client.cc | 19 +++++++------------ src/store_rebuild.cc | 15 ++++++--------- src/ufsdump.cc | 4 ++-- 6 files changed, 32 insertions(+), 39 deletions(-) diff --git a/src/StoreMetaUnpacker.cc b/src/StoreMetaUnpacker.cc index 5b5cfb737d..5bb86796be 100644 --- 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(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 23f8f8a1d0..a7a2be8818 100644 --- 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 833ac5bb35..981ae8281c 100644 --- 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 1d4b67eeca..0542371517 100644 --- 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 178c054eb4..635b4adbcf 100644 --- 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 9bbb9d72e6..fa895ef733 100644 --- 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); -- 2.47.2