From a87fe66d1a7654b8f2ee2b7fe27687d582230888 Mon Sep 17 00:00:00 2001 From: Abhijat Malviya Date: Thu, 20 Mar 2025 14:58:14 +0530 Subject: [PATCH] fix(hset_family): Fix crash on scan after expiry set (#4802) When expiry is set on an hset its members are migrated to a string map. If a scan is performed on the hset, the code branch for string map accessed the value pointer directly by loading the 64 bit value from the object pointer. When creating a new entry in string map with TTL, we also set a bit on the stored pointer as metadata, this is set as 1 << 63. As a result when loading back the pointer we need to unset the same bit to get the correct address, this is done already in string map internals but the scan code did not do this, resulting in segfault. This change adds an AND operation to unset the TTL bit before dereferencing the pointer. Signed-off-by: Abhijat Malviya --- src/core/string_map.cc | 12 ++++++------ src/core/string_map.h | 2 ++ src/server/hset_family.cc | 3 +-- src/server/hset_family_test.cc | 14 ++++++++++++++ 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/core/string_map.cc b/src/core/string_map.cc index 709934447..0f86acdb2 100644 --- a/src/core/string_map.cc +++ b/src/core/string_map.cc @@ -22,12 +22,6 @@ namespace { constexpr uint64_t kValTtlBit = 1ULL << 63; constexpr uint64_t kValMask = ~kValTtlBit; -inline sds GetValue(sds key) { - char* valptr = key + sdslen(key) + 1; - uint64_t val = absl::little_endian::Load64(valptr); - return (sds)(kValMask & val); -} - // Returns key, tagged value pair pair CreateEntry(string_view field, string_view value, uint32_t time_now, uint32_t ttl_sec) { @@ -189,6 +183,12 @@ void StringMap::RandomPairs(unsigned int count, std::vector& keys, std::vec } } +sds StringMap::GetValue(sds key) { + char* valptr = key + sdslen(key) + 1; + const uint64_t val = absl::little_endian::Load64(valptr); + return (sds)(kValMask & val); +} + pair StringMap::ReallocIfNeeded(void* obj, float ratio) { sds key = (sds)obj; size_t key_len = sdslen(key); diff --git a/src/core/string_map.h b/src/core/string_map.h index 59c3c3534..4b8caaa47 100644 --- a/src/core/string_map.h +++ b/src/core/string_map.h @@ -147,6 +147,8 @@ class StringMap : public DenseSet { void RandomPairs(unsigned int count, std::vector& keys, std::vector& vals, bool with_value); + static sds GetValue(sds key); + private: // Reallocate key and/or value if their pages are underutilized. // Returns new pointer (stays same if key utilization is enough) and if reallocation happened. diff --git a/src/server/hset_family.cc b/src/server/hset_family.cc index 821ac6375..15993d084 100644 --- a/src/server/hset_family.cc +++ b/src/server/hset_family.cc @@ -320,8 +320,7 @@ OpResult OpScan(const OpArgs& op_args, std::string_view key, uint64_t size_t len = sdslen(val); if (scan_op.Matches(string_view(val, len))) { res.emplace_back(val, len); - val += (len + 1); - val = (sds)absl::little_endian::Load64(val); + val = StringMap::GetValue(val); res.emplace_back(val, sdslen(val)); } }; diff --git a/src/server/hset_family_test.cc b/src/server/hset_family_test.cc index 4e3190e02..bd21fa7a9 100644 --- a/src/server/hset_family_test.cc +++ b/src/server/hset_family_test.cc @@ -507,4 +507,18 @@ TEST_F(HSetFamilyTest, EmptyHashBug) { EXPECT_THAT(Run({"EXISTS", "foo"}), IntArg(0)); } +TEST_F(HSetFamilyTest, ScanAfterExpireSet) { + EXPECT_THAT(Run({"HSET", "aset", "afield", "avalue"}), IntArg(1)); + EXPECT_THAT(Run({"HEXPIRE", "aset", "1", "FIELDS", "1", "afield"}), IntArg(1)); + + const auto resp = Run({"HSCAN", "aset", "0", "count", "100"}); + EXPECT_THAT(resp, ArrLen(2)); + + const auto vec = StrArray(resp.GetVec()[1]); + EXPECT_EQ(vec.size(), 2); + + EXPECT_THAT(vec, Contains("afield").Times(1)); + EXPECT_THAT(vec, Contains("avalue").Times(1)); +} + } // namespace dfly