From b235617a0d1c2b01e53520e8155ae5680636b92a Mon Sep 17 00:00:00 2001 From: Stepan Bagritsevich <43710058+BagritsevichStepan@users.noreply.github.com> Date: Wed, 18 Sep 2024 14:31:17 +0200 Subject: [PATCH] fix(json_family): Fix out of bound ranges for the JSON.ARR* commands (#3712) * fix(json_family): Fix out of bound ranges for theJSON.ARR* commands Signed-off-by: Stepan Bagritsevich * refactor(json_family): address comments Signed-off-by: Stepan Bagritsevich * refactor(json_family): address comments 2 Signed-off-by: Stepan Bagritsevich --------- Signed-off-by: Stepan Bagritsevich --- src/server/json_family.cc | 128 ++++++++++++++-------------- src/server/json_family_test.cc | 147 +++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 68 deletions(-) diff --git a/src/server/json_family.cc b/src/server/json_family.cc index 4816a3d92..8d93c4118 100644 --- a/src/server/json_family.cc +++ b/src/server/json_family.cc @@ -310,6 +310,25 @@ facade::OpStatus SetJson(const OpArgs& op_args, string_view key, JsonType&& valu return OpStatus::OK; } +size_t NormalizeNegativeIndex(int index, size_t size) { + if (index >= 0) { + return index; + } + + if (static_cast(-index) > size) { + return 0; + } + return size + index; +} + +auto GetJsonArrayIterator(JsonType* val, size_t index) { + return std::next(val->array_range().begin(), static_cast(index)); +} + +auto GetJsonArrayIterator(const JsonType& val, size_t index) { + return std::next(val.array_range().begin(), static_cast(index)); +} + string JsonTypeToName(const JsonType& val) { using namespace std::string_literals; @@ -974,19 +993,10 @@ auto OpArrPop(const OpArgs& op_args, string_view key, WrappedJsonPath& path, int return {}; } - size_t removal_index; - if (index < 0) { - int temp_index = index + val->size(); - removal_index = abs(temp_index); - } else { - removal_index = index; - } + size_t array_size = val->size(); + size_t removal_index = std::min(NormalizeNegativeIndex(index, array_size), array_size - 1); - if (removal_index >= val->size()) { - removal_index %= val->size(); // rounded to the array boundaries. - } - - auto it = val->array_range().begin() + removal_index; + auto it = GetJsonArrayIterator(val, removal_index); string str; error_code ec; it->dump(str, {}, ec); @@ -1014,29 +1024,23 @@ auto OpArrTrim(const OpArgs& op_args, string_view key, const WrappedJsonPath& pa if (val->empty()) { return {false, 0}; } - size_t trim_start_index; - if (start_index < 0) { - trim_start_index = 0; - } else { - trim_start_index = start_index; - } - size_t trim_end_index; - if ((size_t)stop_index >= val->size()) { - trim_end_index = val->size(); - } else { - trim_end_index = stop_index; - } + size_t array_size = val->size(); - if (trim_start_index >= val->size() || trim_start_index > trim_end_index) { + size_t trim_start_index = NormalizeNegativeIndex(start_index, array_size); + size_t trim_end_index = NormalizeNegativeIndex(stop_index, array_size); + + if (trim_start_index >= array_size || trim_start_index > trim_end_index) { val->erase(val->array_range().begin(), val->array_range().end()); return {false, 0}; } - auto trim_start_it = std::next(val->array_range().begin(), trim_start_index); + trim_end_index = std::min(trim_end_index, array_size); + + auto trim_start_it = GetJsonArrayIterator(val, trim_start_index); auto trim_end_it = val->array_range().end(); if (trim_end_index < val->size()) { - trim_end_it = std::next(val->array_range().begin(), trim_end_index + 1); + trim_end_it = GetJsonArrayIterator(val, trim_end_index + 1); } *val = jsoncons::json_array(trim_start_it, trim_end_it); @@ -1055,38 +1059,28 @@ OpResult> OpArrInsert(const OpArgs& op_args, string_ // If at least one index isn't valid within an array in the json doc, the operation is discarded. // Negative indexes start from the end of the array. auto cb = [&](std::optional, JsonType* val) -> MutateCallbackResult { - if (out_of_boundaries_encountered) { + if (out_of_boundaries_encountered || !val->is_array()) { return {}; } - if (!val->is_array()) { - return {}; - } + size_t array_size = val->size(); + size_t insert_before_index; - size_t removal_index; if (index < 0) { - if (val->empty()) { + if (static_cast(-index) > array_size) { out_of_boundaries_encountered = true; return {}; } - - int temp_index = index + val->size(); - if (temp_index < 0) { - out_of_boundaries_encountered = true; - return {}; - } - - removal_index = temp_index; + insert_before_index = array_size + index; } else { - if ((size_t)index > val->size()) { + if (static_cast(index) > val->size()) { out_of_boundaries_encountered = true; return {}; } - - removal_index = index; + insert_before_index = index; } - auto it = next(val->array_range().begin(), removal_index); + auto it = GetJsonArrayIterator(val, insert_before_index); for (auto& new_val : new_values) { it = val->insert(it, new_val); it++; @@ -1130,38 +1124,41 @@ auto OpArrIndex(const OpArgs& op_args, string_view key, const WrappedJsonPath& j return -1; } - // Negative value or out-of-range index is handled by rounding the index to the array's start - // and end. example: for array size 9 and index -11 the index mapped to index 7. - if (start_index < 0) { - start_index %= val.size(); - start_index += val.size(); + size_t array_size = val.size(); + + if (start_index < 0 && static_cast(-start_index) > array_size) { + return -1; } - // See the comment above. - // A value index of 0 means searching until the end of the array. - if (end_index == 0) { - end_index = val.size(); - } else if (end_index < 0) { - end_index %= val.size(); - end_index += val.size(); + size_t pos_start_index = NormalizeNegativeIndex(start_index, array_size); + size_t pos_end_index = + end_index == 0 ? array_size : NormalizeNegativeIndex(end_index, array_size); + + if (pos_start_index >= array_size && pos_end_index < array_size) { + return -1; } - if (start_index > end_index) { + pos_start_index = std::min(pos_start_index, array_size - 1); + pos_end_index = std::min(pos_end_index, array_size - 1); + + if (pos_start_index > pos_end_index) { return -1; } size_t pos = -1; - auto it = next(val.array_range().begin(), start_index); + auto it = GetJsonArrayIterator(val, pos_start_index); while (it != val.array_range().end()) { if (JsonAreEquals(search_val, *it)) { - pos = start_index; + pos = pos_start_index; + break; + } + + if (pos_start_index == pos_end_index) { break; } ++it; - if (++start_index == end_index) { - break; - } + pos_start_index++; } return pos; @@ -1690,11 +1687,6 @@ void JsonFamily::ArrTrim(CmdArgList args, ConnectionContext* cntx) { return; } - if (stop_index < 0) { - cntx->SendError(kInvalidIntErr); - return; - } - WrappedJsonPath json_path = GET_OR_SEND_UNEXPECTED(ParseJsonPath(path)); auto cb = [&](Transaction* t, EngineShard* shard) { diff --git a/src/server/json_family_test.cc b/src/server/json_family_test.cc index f47d4b1db..0a449a4d7 100644 --- a/src/server/json_family_test.cc +++ b/src/server/json_family_test.cc @@ -1922,6 +1922,37 @@ TEST_F(JsonFamilyTest, ArrPopLegacy) { EXPECT_THAT(resp, ArgType(RespExpr::NIL)); } +TEST_F(JsonFamilyTest, ArrPopOutOfRange) { + string json = R"( + [0,1,2,3,4,5] + )"; + + auto resp = Run({"JSON.SET", "arr", "$", json}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRPOP", "arr", "$", "-55"}); + EXPECT_EQ(resp, "0"); + + resp = Run({"JSON.SET", "arr", "$", json}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRPOP", "arr", "$", "55"}); + EXPECT_EQ(resp, "5"); + + // Test legacy mode + resp = Run({"JSON.SET", "arr", ".", json}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRPOP", "arr", ".", "-55"}); + EXPECT_EQ(resp, "0"); + + resp = Run({"JSON.SET", "arr", ".", json}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRPOP", "arr", ".", "55"}); + EXPECT_EQ(resp, "5"); +} + TEST_F(JsonFamilyTest, ArrTrim) { string json = R"( [[], ["a"], ["a", "b"], ["a", "b", "c"]] @@ -2043,6 +2074,54 @@ TEST_F(JsonFamilyTest, ArrTrimLegacy) { EXPECT_THAT(resp, ErrArg("wrong JSON type of path value")); } +TEST_F(JsonFamilyTest, ArrTrimOutOfRange) { + string arr = R"( + [0,1,2,3,4] + )"; + + auto resp = Run({"JSON.SET", "arr", "$", arr}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRTRIM", "arr", "$", "-1", "3"}); + EXPECT_THAT(resp, IntArg(0)); + EXPECT_EQ(Run({"JSON.GET", "arr"}), "[]"); + + resp = Run({"JSON.SET", "arr", "$", arr}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRTRIM", "arr", "$", "54", "55"}); + EXPECT_THAT(resp, IntArg(0)); + EXPECT_EQ(Run({"JSON.GET", "arr"}), "[]"); + + resp = Run({"JSON.SET", "arr", "$", arr}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRTRIM", "arr", "$", "56", "55"}); + EXPECT_THAT(resp, IntArg(0)); + EXPECT_EQ(Run({"JSON.GET", "arr"}), "[]"); + + resp = Run({"JSON.SET", "arr", "$", arr}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRTRIM", "arr", "$", "-55", "-55"}); + EXPECT_THAT(resp, IntArg(1)); + EXPECT_EQ(Run({"JSON.GET", "arr"}), "[0]"); + + resp = Run({"JSON.SET", "arr", "$", arr}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRTRIM", "arr", "$", "-2", "-1"}); + EXPECT_THAT(resp, IntArg(2)); + EXPECT_EQ(Run({"JSON.GET", "arr"}), "[3,4]"); + + resp = Run({"JSON.SET", "arr", "$", arr}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRTRIM", "arr", "$", "-1", "-2"}); + EXPECT_THAT(resp, IntArg(0)); + EXPECT_EQ(Run({"JSON.GET", "arr"}), "[]"); +} + TEST_F(JsonFamilyTest, ArrInsert) { string json = R"( [[], ["a"], ["a", "b"]] @@ -2111,6 +2190,42 @@ TEST_F(JsonFamilyTest, ArrInsertLegacy) { EXPECT_THAT(resp, ErrArg("wrong JSON type of path value")); } +TEST_F(JsonFamilyTest, ArrInsertOutOfRange) { + string json = R"( + [0,1,2,3,4,5] + )"; + + auto resp = Run({"JSON.SET", "arr", ".", json}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRINSERT", "arr", "$", "-55", "6"}); + EXPECT_THAT(resp, ErrArg("index out of range")); + + resp = Run({"JSON.ARRINSERT", "arr", "$", "55", "6"}); + EXPECT_THAT(resp, ErrArg("index out of range")); + + resp = Run({"JSON.ARRINSERT", "arr", ".", "-55", "6"}); // Legacy mode + EXPECT_THAT(resp, ErrArg("index out of range")); + + resp = Run({"JSON.ARRINSERT", "arr", ".", "55", "6"}); // Legacy mode + EXPECT_THAT(resp, ErrArg("index out of range")); + + resp = Run({"JSON.SET", "arr", ".", "[]"}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRINSERT", "arr", "$", "-1", "2"}); + EXPECT_THAT(resp, ErrArg("index out of range")); + + resp = Run({"JSON.ARRINSERT", "arr", "$", "1", "2"}); + EXPECT_THAT(resp, ErrArg("index out of range")); + + resp = Run({"JSON.ARRINSERT", "arr", "$", "0", "2"}); + EXPECT_THAT(resp, IntArg(1)); + + resp = Run({"JSON.GET", "arr"}); + EXPECT_EQ(resp, "[2]"); +} + TEST_F(JsonFamilyTest, ArrAppend) { string json = R"( [[], ["a"], ["a", "b"]] @@ -2332,6 +2447,38 @@ TEST_F(JsonFamilyTest, ArrIndexWithNumericValuesLegacy) { EXPECT_THAT(resp, IntArg(-1)); } +TEST_F(JsonFamilyTest, ArrIndexOutOfRange) { + auto resp = Run({"JSON.SET", "arr", ".", R"([1,1,1,1,1])"}); + ASSERT_THAT(resp, "OK"); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "-55", "-55"}); + EXPECT_THAT(resp, IntArg(-1)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "-55", "-56"}); + EXPECT_THAT(resp, IntArg(-1)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "-55", "-54"}); + EXPECT_THAT(resp, IntArg(-1)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "-2"}); + EXPECT_THAT(resp, IntArg(3)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "-2", "-1"}); + EXPECT_THAT(resp, IntArg(3)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "-2", "-3"}); + EXPECT_THAT(resp, IntArg(-1)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "55", "56"}); + EXPECT_THAT(resp, IntArg(4)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "55", "54"}); + EXPECT_THAT(resp, IntArg(4)); + + resp = Run({"JSON.ARRINDEX", "arr", "$", "1", "5", "4"}); + EXPECT_THAT(resp, IntArg(-1)); +} + TEST_F(JsonFamilyTest, MGet) { string json[] = { R"(