From 023aa7c89e8a5f66b6421bbd185cfacf13f3f08d 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 8d34d14f6..e6cd9f343 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) { @@ -184,6 +178,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 567fe1727..086e26117 100644 --- a/src/core/string_map.h +++ b/src/core/string_map.h @@ -146,6 +146,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 9a3d63287..0fba8a759 100644 --- a/src/server/hset_family.cc +++ b/src/server/hset_family.cc @@ -329,8 +329,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 0c8bc031c..f6b981c0c 100644 --- a/src/server/hset_family_test.cc +++ b/src/server/hset_family_test.cc @@ -456,4 +456,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