Avoid UB in LRUHandle key storage

util/cache.cc

@@ -48,17 +48,22 @@ struct LRUHandle {
   LRUHandle* prev;
   size_t charge;  // TODO(opt): Only allow uint32_t?
   size_t key_length;
-  bool in_cache;     // Whether entry is in the cache.
-  uint32_t refs;     // References, including cache reference, if present.
-  uint32_t hash;     // Hash of key(); used for fast sharding and comparisons
-  char key_data[1];  // Beginning of key
+  bool in_cache;  // Whether entry is in the cache.
+  uint32_t refs;  // References, including cache reference, if present.
+  uint32_t hash;  // Hash of key(); used for fast sharding and comparisons
 
   Slice key() const {
     // next is only equal to this if the LRU handle is the list head of an
     // empty list. List heads never have meaningful keys.
     assert(next != this);
 
-    return Slice(key_data, key_length);
+    return Slice(key_data(), key_length);
+  }
+
+  char* key_data() { return reinterpret_cast<char*>(this + 1); }
+
+  const char* key_data() const {
+    return reinterpret_cast<const char*>(this + 1);
   }
 };
 
@@ -271,15 +276,15 @@ Cache::Handle* LRUCache::Insert(const Slice& key, uint32_t hash, void* value,
   MutexLock l(&mutex_);
 
   LRUHandle* e =
-      reinterpret_cast<LRUHandle*>(malloc(sizeof(LRUHandle) - 1 + key.size()));
+      reinterpret_cast<LRUHandle*>(malloc(sizeof(LRUHandle) + key.size()));
   e->value = value;
   e->deleter = deleter;
   e->charge = charge;
   e->key_length = key.size();
   e->hash = hash;
   e->in_cache = false;
   e->refs = 1;  // for the returned handle.
-  std::memcpy(e->key_data, key.data(), key.size());
+  std::memcpy(e->key_data(), key.data(), key.size());
 
   if (capacity_ > 0) {
     e->refs++;  // for the cache's reference.

util/cache_test.cc

@@ -4,11 +4,13 @@
 
 #include "leveldb/cache.h"
 
+#include <string>
 #include <vector>
 
-#include "gtest/gtest.h"
 #include "util/coding.h"
 
+#include "gtest/gtest.h"
+
 namespace leveldb {
 
 // Conversions between numeric keys/values and the types expected by Cache.
@@ -31,8 +33,14 @@ class CacheTest : public testing::Test {
     current_->deleted_values_.push_back(DecodeValue(v));
   }
 
+  static void StringKeyDeleter(const Slice& key, void* v) {
+    current_->deleted_string_keys_.push_back(key.ToString());
+    current_->deleted_values_.push_back(DecodeValue(v));
+  }
+
   static constexpr int kCacheSize = 1000;
   std::vector<int> deleted_keys_;
+  std::vector<std::string> deleted_string_keys_;
   std::vector<int> deleted_values_;
   Cache* cache_;
 
@@ -87,6 +95,26 @@ TEST_F(CacheTest, HitAndMiss) {
   ASSERT_EQ(101, deleted_values_[0]);
 }
 
+TEST_F(CacheTest, InsertKeyLongerThanOneByte) {
+  const std::string key = "long-key";
+  Cache::Handle* handle =
+      cache_->Insert(key, EncodeValue(101), 1, &CacheTest::StringKeyDeleter);
+  ASSERT_NE(nullptr, handle);
+  ASSERT_EQ(101, DecodeValue(cache_->Value(handle)));
+  cache_->Release(handle);
+
+  handle = cache_->Lookup(key);
+  ASSERT_NE(nullptr, handle);
+  ASSERT_EQ(101, DecodeValue(cache_->Value(handle)));
+  cache_->Release(handle);
+
+  cache_->Erase(key);
+  ASSERT_EQ(1, deleted_string_keys_.size());
+  ASSERT_EQ(key, deleted_string_keys_[0]);
+  ASSERT_EQ(1, deleted_values_.size());
+  ASSERT_EQ(101, deleted_values_[0]);
+}
+
 TEST_F(CacheTest, Erase) {
   Erase(200);
   ASSERT_EQ(0, deleted_keys_.size());