From 89e8c02fac863b66974dce1a242796c1972436a9 Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sun, 14 Jan 2024 10:18:14 +0800 Subject: [PATCH 1/7] check type before decode metadata --- src/storage/redis_metadata.cc | 54 +++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/storage/redis_metadata.cc b/src/storage/redis_metadata.cc index 23afc765457..78658385a0e 100644 --- a/src/storage/redis_metadata.cc +++ b/src/storage/redis_metadata.cc @@ -372,29 +372,31 @@ rocksdb::Status StreamMetadata::Decode(Slice *input) { return s; } - if (input->size() < 8 * 11) { - return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); - } + if (Type() == kRedisStream) { + if (input->size() < 8 * 11) { + return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); + } - GetFixed64(input, &last_generated_id.ms); - GetFixed64(input, &last_generated_id.seq); + GetFixed64(input, &last_generated_id.ms); + GetFixed64(input, &last_generated_id.seq); - GetFixed64(input, &recorded_first_entry_id.ms); - GetFixed64(input, &recorded_first_entry_id.seq); + GetFixed64(input, &recorded_first_entry_id.ms); + GetFixed64(input, &recorded_first_entry_id.seq); - GetFixed64(input, &max_deleted_entry_id.ms); - GetFixed64(input, &max_deleted_entry_id.seq); + GetFixed64(input, &max_deleted_entry_id.ms); + GetFixed64(input, &max_deleted_entry_id.seq); - GetFixed64(input, &first_entry_id.ms); - GetFixed64(input, &first_entry_id.seq); + GetFixed64(input, &first_entry_id.ms); + GetFixed64(input, &first_entry_id.seq); - GetFixed64(input, &last_entry_id.ms); - GetFixed64(input, &last_entry_id.seq); + GetFixed64(input, &last_entry_id.ms); + GetFixed64(input, &last_entry_id.seq); - GetFixed64(input, &entries_added); + GetFixed64(input, &entries_added); - if (input->size() >= 8) { - GetFixed64(input, &group_number); + if (input->size() >= 8) { + GetFixed64(input, &group_number); + } } return rocksdb::Status::OK(); @@ -416,16 +418,18 @@ rocksdb::Status BloomChainMetadata::Decode(Slice *input) { return s; } - if (input->size() < 20) { - return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); - } + if (Type() == kRedisBloomFilter) { + if (input->size() < 20) { + return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); + } - GetFixed16(input, &n_filters); - GetFixed16(input, &expansion); + GetFixed16(input, &n_filters); + GetFixed16(input, &expansion); - GetFixed32(input, &base_capacity); - GetDouble(input, &error_rate); - GetFixed32(input, &bloom_bytes); + GetFixed32(input, &base_capacity); + GetDouble(input, &error_rate); + GetFixed32(input, &bloom_bytes); + } return rocksdb::Status::OK(); } @@ -454,7 +458,7 @@ rocksdb::Status JsonMetadata::Decode(Slice *input) { return s; } - if (!GetFixed8(input, reinterpret_cast(&format))) { + if (Type() == kRedisJson && !GetFixed8(input, reinterpret_cast(&format))) { return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); } From e417ba4b7f0e8877b68b93eeb448dd9ca6d2564e Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sun, 14 Jan 2024 10:33:01 +0800 Subject: [PATCH 2/7] add test case --- tests/gocase/unit/type/type_test.go | 54 +++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 tests/gocase/unit/type/type_test.go diff --git a/tests/gocase/unit/type/type_test.go b/tests/gocase/unit/type/type_test.go new file mode 100644 index 00000000000..d6d279dfa47 --- /dev/null +++ b/tests/gocase/unit/type/type_test.go @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package types + +import ( + "context" + "testing" + + "github.com/apache/kvrocks/tests/gocase/util" + "github.com/stretchr/testify/require" +) + +func TestTypesError(t *testing.T) { + srv := util.StartServer(t, map[string]string{}) + defer srv.Close() + ctx := context.Background() + rdb := srv.NewClient() + defer func() { require.NoError(t, rdb.Close()) }() + + t.Run("Operation Wrong Type", func(t *testing.T) { + MESSAGE := "ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value" + require.NoError(t, rdb.Set(ctx, "a", "hello", 0).Err()) + require.EqualError(t, rdb.Do(ctx, "XADD", "a", "*", "a", "test").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "LPUSH", "a", 1).Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "HSET", "a", "1", "2").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "SADD", "a", "1", "2").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "ZADD", "a", "1", "2").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "JSON.SET", "a", "$", "{}").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "BF.ADD", "a", "test").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "SADD", "a", 100).Err(), MESSAGE) + + require.NoError(t, rdb.LPush(ctx, "a1", "hello", 0).Err()) + require.EqualError(t, rdb.Do(ctx, "SETBIT", "a1", 1, 1).Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "GET", "a1").Err(), MESSAGE) + + }) +} From d2d11a86942895ad2405c87183ce2859cac7be06 Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sun, 14 Jan 2024 10:33:48 +0800 Subject: [PATCH 3/7] format --- tests/gocase/unit/type/type_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/gocase/unit/type/type_test.go b/tests/gocase/unit/type/type_test.go index d6d279dfa47..a39a1db4207 100644 --- a/tests/gocase/unit/type/type_test.go +++ b/tests/gocase/unit/type/type_test.go @@ -35,20 +35,20 @@ func TestTypesError(t *testing.T) { defer func() { require.NoError(t, rdb.Close()) }() t.Run("Operation Wrong Type", func(t *testing.T) { - MESSAGE := "ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value" + message := "ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value" require.NoError(t, rdb.Set(ctx, "a", "hello", 0).Err()) - require.EqualError(t, rdb.Do(ctx, "XADD", "a", "*", "a", "test").Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "LPUSH", "a", 1).Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "HSET", "a", "1", "2").Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "SADD", "a", "1", "2").Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "ZADD", "a", "1", "2").Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "JSON.SET", "a", "$", "{}").Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "BF.ADD", "a", "test").Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "SADD", "a", 100).Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "XADD", "a", "*", "a", "test").Err(), message) + require.EqualError(t, rdb.Do(ctx, "LPUSH", "a", 1).Err(), message) + require.EqualError(t, rdb.Do(ctx, "HSET", "a", "1", "2").Err(), message) + require.EqualError(t, rdb.Do(ctx, "SADD", "a", "1", "2").Err(), message) + require.EqualError(t, rdb.Do(ctx, "ZADD", "a", "1", "2").Err(), message) + require.EqualError(t, rdb.Do(ctx, "JSON.SET", "a", "$", "{}").Err(), message) + require.EqualError(t, rdb.Do(ctx, "BF.ADD", "a", "test").Err(), message) + require.EqualError(t, rdb.Do(ctx, "SADD", "a", 100).Err(), message) require.NoError(t, rdb.LPush(ctx, "a1", "hello", 0).Err()) - require.EqualError(t, rdb.Do(ctx, "SETBIT", "a1", 1, 1).Err(), MESSAGE) - require.EqualError(t, rdb.Do(ctx, "GET", "a1").Err(), MESSAGE) + require.EqualError(t, rdb.Do(ctx, "SETBIT", "a1", 1, 1).Err(), message) + require.EqualError(t, rdb.Do(ctx, "GET", "a1").Err(), message) }) } From 34b00b47ba2ce83095637fa28c755cdec5796363 Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sun, 14 Jan 2024 12:52:05 +0800 Subject: [PATCH 4/7] rename test file --- tests/gocase/unit/type/{type_test.go => types_test.go} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/gocase/unit/type/{type_test.go => types_test.go} (97%) diff --git a/tests/gocase/unit/type/type_test.go b/tests/gocase/unit/type/types_test.go similarity index 97% rename from tests/gocase/unit/type/type_test.go rename to tests/gocase/unit/type/types_test.go index a39a1db4207..3a33c1d56c2 100644 --- a/tests/gocase/unit/type/type_test.go +++ b/tests/gocase/unit/type/types_test.go @@ -34,7 +34,7 @@ func TestTypesError(t *testing.T) { rdb := srv.NewClient() defer func() { require.NoError(t, rdb.Close()) }() - t.Run("Operation Wrong Type", func(t *testing.T) { + t.Run("Operate with wrong type", func(t *testing.T) { message := "ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value" require.NoError(t, rdb.Set(ctx, "a", "hello", 0).Err()) require.EqualError(t, rdb.Do(ctx, "XADD", "a", "*", "a", "test").Err(), message) From 035801c519cdeb677e26627bf32e525a87f2dbbd Mon Sep 17 00:00:00 2001 From: jihuayu Date: Tue, 16 Jan 2024 15:41:20 +0800 Subject: [PATCH 5/7] move type check to Database::ParseMetadata --- src/storage/redis_db.cc | 2 +- src/storage/redis_metadata.cc | 66 ++++++++++++++++------------------- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/src/storage/redis_db.cc b/src/storage/redis_db.cc index bdbd9789490..ebbcbb06cb7 100644 --- a/src/storage/redis_db.cc +++ b/src/storage/redis_db.cc @@ -55,7 +55,7 @@ rocksdb::Status Database::ParseMetadata(RedisTypes types, Slice *bytes, Metadata }); auto s = metadata->Decode(bytes); - if (!s.ok()) return s; + if (!s.ok() && !s.IsInvalidArgument()) return s; if (metadata->Expired()) { // error discarded here since it already failed diff --git a/src/storage/redis_metadata.cc b/src/storage/redis_metadata.cc index 78658385a0e..458ab4e880e 100644 --- a/src/storage/redis_metadata.cc +++ b/src/storage/redis_metadata.cc @@ -335,13 +335,13 @@ rocksdb::Status ListMetadata::Decode(Slice *input) { if (auto s = Metadata::Decode(input); !s.ok()) { return s; } - if (Type() == kRedisList) { - if (input->size() < 8 + 8) { - return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); - } - GetFixed64(input, &head); - GetFixed64(input, &tail); + + if (input->size() < 8 + 8) { + return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); } + GetFixed64(input, &head); + GetFixed64(input, &tail); + return rocksdb::Status::OK(); } @@ -372,31 +372,29 @@ rocksdb::Status StreamMetadata::Decode(Slice *input) { return s; } - if (Type() == kRedisStream) { - if (input->size() < 8 * 11) { - return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); - } + if (input->size() < 8 * 11) { + return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); + } - GetFixed64(input, &last_generated_id.ms); - GetFixed64(input, &last_generated_id.seq); + GetFixed64(input, &last_generated_id.ms); + GetFixed64(input, &last_generated_id.seq); - GetFixed64(input, &recorded_first_entry_id.ms); - GetFixed64(input, &recorded_first_entry_id.seq); + GetFixed64(input, &recorded_first_entry_id.ms); + GetFixed64(input, &recorded_first_entry_id.seq); - GetFixed64(input, &max_deleted_entry_id.ms); - GetFixed64(input, &max_deleted_entry_id.seq); + GetFixed64(input, &max_deleted_entry_id.ms); + GetFixed64(input, &max_deleted_entry_id.seq); - GetFixed64(input, &first_entry_id.ms); - GetFixed64(input, &first_entry_id.seq); + GetFixed64(input, &first_entry_id.ms); + GetFixed64(input, &first_entry_id.seq); - GetFixed64(input, &last_entry_id.ms); - GetFixed64(input, &last_entry_id.seq); + GetFixed64(input, &last_entry_id.ms); + GetFixed64(input, &last_entry_id.seq); - GetFixed64(input, &entries_added); + GetFixed64(input, &entries_added); - if (input->size() >= 8) { - GetFixed64(input, &group_number); - } + if (input->size() >= 8) { + GetFixed64(input, &group_number); } return rocksdb::Status::OK(); @@ -418,18 +416,16 @@ rocksdb::Status BloomChainMetadata::Decode(Slice *input) { return s; } - if (Type() == kRedisBloomFilter) { - if (input->size() < 20) { - return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); - } + if (input->size() < 20) { + return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); + } - GetFixed16(input, &n_filters); - GetFixed16(input, &expansion); + GetFixed16(input, &n_filters); + GetFixed16(input, &expansion); - GetFixed32(input, &base_capacity); - GetDouble(input, &error_rate); - GetFixed32(input, &bloom_bytes); - } + GetFixed32(input, &base_capacity); + GetDouble(input, &error_rate); + GetFixed32(input, &bloom_bytes); return rocksdb::Status::OK(); } @@ -458,7 +454,7 @@ rocksdb::Status JsonMetadata::Decode(Slice *input) { return s; } - if (Type() == kRedisJson && !GetFixed8(input, reinterpret_cast(&format))) { + if (!GetFixed8(input, reinterpret_cast(&format))) { return rocksdb::Status::InvalidArgument(kErrMetadataTooShort); } From 4e4d0ccd46ff172f01d4a175bfc2cdc814e11126 Mon Sep 17 00:00:00 2001 From: jihuayu Date: Tue, 16 Jan 2024 15:45:21 +0800 Subject: [PATCH 6/7] delay InvalidArgument error check after type match check --- src/storage/redis_db.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/storage/redis_db.cc b/src/storage/redis_db.cc index ebbcbb06cb7..00df708c9d1 100644 --- a/src/storage/redis_db.cc +++ b/src/storage/redis_db.cc @@ -55,6 +55,7 @@ rocksdb::Status Database::ParseMetadata(RedisTypes types, Slice *bytes, Metadata }); auto s = metadata->Decode(bytes); + // delay InvalidArgument error check after type match check if (!s.ok() && !s.IsInvalidArgument()) return s; if (metadata->Expired()) { @@ -69,6 +70,8 @@ rocksdb::Status Database::ParseMetadata(RedisTypes types, Slice *bytes, Metadata auto _ [[maybe_unused]] = metadata->Decode(old_metadata); return rocksdb::Status::InvalidArgument(kErrMsgWrongType); } + if (s.IsInvalidArgument()) return s; + if (metadata->size == 0 && !metadata->IsEmptyableType()) { // error discarded here since it already failed auto _ [[maybe_unused]] = metadata->Decode(old_metadata); From 076b18b5bbbff194e44c438f50529b2cb8defaa5 Mon Sep 17 00:00:00 2001 From: jihuayu Date: Tue, 16 Jan 2024 15:51:37 +0800 Subject: [PATCH 7/7] format code --- src/storage/redis_db.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/redis_db.cc b/src/storage/redis_db.cc index 00df708c9d1..eaa94b431ee 100644 --- a/src/storage/redis_db.cc +++ b/src/storage/redis_db.cc @@ -71,7 +71,7 @@ rocksdb::Status Database::ParseMetadata(RedisTypes types, Slice *bytes, Metadata return rocksdb::Status::InvalidArgument(kErrMsgWrongType); } if (s.IsInvalidArgument()) return s; - + if (metadata->size == 0 && !metadata->IsEmptyableType()) { // error discarded here since it already failed auto _ [[maybe_unused]] = metadata->Decode(old_metadata);