From 5689f3b6df62b37a61a11eeb1a1158c1980c3f79 Mon Sep 17 00:00:00 2001 From: Binbin Date: Wed, 5 Jul 2023 18:09:34 +0800 Subject: [PATCH] Fix GETEX not checking wrong type error causing key overwriting This bug causes keys to be overwritten: ``` 127.0.0.1:6666> lpush foo bar (integer) 1 127.0.0.1:6666> type foo list 127.0.0.1:6666> getex foo "" 127.0.0.1:6666> type foo string ``` The reason is that we did not return early for wrong type error and then the code overwrites it as a string key. We should throw a wrong type error in this case: ``` 127.0.0.1:6666> lpush foo bar (integer) 1 127.0.0.1:6666> getex foo (error) ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value 127.0.0.1:6666> type foo list ``` GETEX was added in #961, look like this bug has existed since the command was added. --- src/types/redis_string.cc | 2 +- tests/gocase/unit/type/strings/strings_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 7a39cef6abb..f75fe6bb852 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -163,7 +163,7 @@ rocksdb::Status String::GetEx(const std::string &user_key, std::string *value, u LockGuard guard(storage_->GetLockManager(), ns_key); rocksdb::Status s = getValue(ns_key, value); - if (!s.ok() && s.IsNotFound()) return s; + if (!s.ok()) return s; std::string raw_data; Metadata metadata(kRedisString, false); diff --git a/tests/gocase/unit/type/strings/strings_test.go b/tests/gocase/unit/type/strings/strings_test.go index ac36fa5aef8..4b7bf4b28d4 100644 --- a/tests/gocase/unit/type/strings/strings_test.go +++ b/tests/gocase/unit/type/strings/strings_test.go @@ -241,6 +241,14 @@ func TestString(t *testing.T) { util.ErrorRegexp(t, rdb.Do(ctx, "getex").Err(), ".*wrong number of arguments*.") }) + t.Run("GETEX against wrong type", func(t *testing.T) { + rdb.Del(ctx, "foo") + rdb.LPush(ctx, "foo", "bar") + util.ErrorRegexp(t, rdb.Do(ctx, "getex", "foo").Err(), ".*WRONGTYPE.*") + require.EqualValues(t, 1, rdb.Exists(ctx, "foo").Val()) + require.Equal(t, "list", rdb.Type(ctx, "foo").Val()) + }) + t.Run("GETDEL command", func(t *testing.T) { require.NoError(t, rdb.Del(ctx, "foo").Err()) require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())