From e6f5a2073c7a6663dea0c1e8213bcfca60f6ea25 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Tue, 3 Sep 2024 12:43:12 +0300 Subject: [PATCH] fix: crash when passing empty arguments (#3627) * fix: crash when passing empty arguments Fix the case where we pass an empty argument, which then is parsed as an empty string view with null pointer. The null pointer is not handled correctly by our low level c code, hence switch to using ""sv for that. * chore: add more list asserts + improve test_hypothesis --------- Signed-off-by: Roman Gershman --- .github/workflows/ci.yml | 2 +- src/facade/cmd_arg_parser.h | 5 +++-- src/server/list_family.cc | 9 +++++++-- src/server/list_family_test.cc | 8 ++++++++ tests/fakeredis/test/test_hypothesis.py | 6 ++++-- 5 files changed, 23 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a1a3ed233..93a0c739a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -106,7 +106,7 @@ jobs: -DCMAKE_C_COMPILER="${{matrix.compiler.c}}" \ -DCMAKE_CXX_COMPILER="${{matrix.compiler.cxx}}" \ -DCMAKE_CXX_COMPILER_LAUNCHER=sccache -DCMAKE_C_COMPILER_LAUNCHER=sccache \ - -DCMAKE_CXX_FLAGS="${{matrix.cxx_flags}}" \ + -DCMAKE_CXX_FLAGS="${{matrix.cxx_flags}}" -DWITH_AWS:BOOL=OFF \ -L cd ${GITHUB_WORKSPACE}/build && pwd du -hcs _deps/ diff --git a/src/facade/cmd_arg_parser.h b/src/facade/cmd_arg_parser.h index 711f6d5aa..4b363aadf 100644 --- a/src/facade/cmd_arg_parser.h +++ b/src/facade/cmd_arg_parser.h @@ -165,9 +165,10 @@ struct CmdArgParser { } std::string_view SafeSV(size_t i) const { + using namespace std::literals::string_view_literals; if (i >= args_.size()) - return ""; - return ToSV(args_[i]); + return ""sv; + return args_[i].empty() ? ""sv : ToSV(args_[i]); } void Report(ErrorType type, size_t idx) { diff --git a/src/server/list_family.cc b/src/server/list_family.cc index 5135c4de8..a9402061a 100644 --- a/src/server/list_family.cc +++ b/src/server/list_family.cc @@ -446,8 +446,10 @@ OpResult OpIndex(const OpArgs& op_args, std::string_view key, long index return str; } -OpResult> OpPos(const OpArgs& op_args, std::string_view key, - std::string_view element, int rank, int count, int max_len) { +OpResult> OpPos(const OpArgs& op_args, string_view key, string_view element, + int rank, int count, int max_len) { + DCHECK(key.data() && element.data()); + auto it_res = op_args.GetDbSlice().FindReadOnly(op_args.db_cntx, key, OBJ_LIST); if (!it_res.ok()) return it_res.status(); @@ -491,6 +493,8 @@ OpResult> OpPos(const OpArgs& op_args, std::string_view key, OpResult OpInsert(const OpArgs& op_args, string_view key, string_view pivot, string_view elem, InsertParam insert_param) { + DCHECK(key.data() && pivot.data() && elem.data()); + auto& db_slice = op_args.GetDbSlice(); auto it_res = db_slice.FindMutable(op_args.db_cntx, key, OBJ_LIST); if (!it_res) @@ -987,6 +991,7 @@ void ListFamily::LInsert(CmdArgList args, ConnectionContext* cntx) { string_view key = parser.Next(); InsertParam where = parser.Switch("AFTER", INSERT_AFTER, "BEFORE", INSERT_BEFORE); auto [pivot, elem] = parser.Next(); + DCHECK(pivot.data() && elem.data()); if (auto err = parser.Error(); err) return cntx->SendError(err->MakeReply()); diff --git a/src/server/list_family_test.cc b/src/server/list_family_test.cc index 9efc10849..a70729d8a 100644 --- a/src/server/list_family_test.cc +++ b/src/server/list_family_test.cc @@ -967,6 +967,14 @@ TEST_F(ListFamilyTest, LInsert) { // Insert after, pivot not found. EXPECT_THAT(Run({"linsert", "mylist", "after", "notfound", "x"}), IntArg(-1)); + + // insert empty + Run({"rpush", "k", "a"}); + Run({"linsert", "k", "before", "a", ""}); + resp = Run({"lpop", "k"}); + EXPECT_EQ(resp, ""); + resp = Run({"linsert", "k", "before", "", ""}); + EXPECT_THAT(resp, IntArg(-1)); } TEST_F(ListFamilyTest, BLPopUnwakesInScript) { diff --git a/tests/fakeredis/test/test_hypothesis.py b/tests/fakeredis/test/test_hypothesis.py index 7ed72befe..3092a96f1 100644 --- a/tests/fakeredis/test/test_hypothesis.py +++ b/tests/fakeredis/test/test_hypothesis.py @@ -49,7 +49,9 @@ int_as_bytes = st.builds(lambda x: str(default_normalize(x)).encode(), st.intege float_as_bytes = st.builds( lambda x: repr(default_normalize(x)).encode(), st.floats(width=32) ) -counts = st.integers(min_value=-3, max_value=3) | st.integers() +counts = st.integers(min_value=-3, max_value=3) | st.integers( + min_value=-2147483648, max_value=2147483647 +) limits = st.just(()) | st.tuples(st.just("limit"), counts, counts) # Redis has an integer overflow bug in swapdb, so we confine the numbers to # a limited range (https://github.com/antirez/redis/issues/5737). @@ -497,7 +499,7 @@ class TestList(BaseTest): | commands( st.sampled_from(["lpop", "rpop"]), keys, - st.just(None) | st.just([]) | st.integers(), + st.just(None) | st.just([]) | counts, ) | commands( st.sampled_from(["lpush", "lpushx", "rpush", "rpushx"]),