From d37a0bbc29950bec42ad607bf067867e50f52bd5 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Wed, 15 Jun 2022 00:43:39 +0300 Subject: [PATCH] fix(zset): Properly convert non-c strings to floats. The old code relied on c strtod function that expected some sort of delimiter at the end. Coincidently my unit-testing code always passed proper c strings so strod worked as expected. EVAL passes slices to non-c string and this is why the bug was discovered via eval call. Fixes #148. Signed-off-by: Roman Gershman --- src/redis/object.c | 21 --------------------- src/redis/util.c | 21 --------------------- src/redis/util.h | 1 - src/server/test_utils.cc | 15 +++++++++++---- src/server/zset_family.cc | 9 +++++---- src/server/zset_family_test.cc | 5 +++++ 6 files changed, 21 insertions(+), 51 deletions(-) diff --git a/src/redis/object.c b/src/redis/object.c index 18b765ff6..4b70097b1 100644 --- a/src/redis/object.c +++ b/src/redis/object.c @@ -658,27 +658,6 @@ size_t stringObjectLen(robj *o) { } } -int getDoubleFromObject(const robj *o, double *target) { - double value; - - if (o == NULL) { - value = 0; - } else { - serverAssertWithInfo(NULL,o,o->type == OBJ_STRING); - if (sdsEncodedObject(o)) { - if (!string2d(o->ptr, sdslen(o->ptr), &value)) - return C_ERR; - } else if (o->encoding == OBJ_ENCODING_INT) { - value = (long)o->ptr; - } else { - serverPanic("Unknown string encoding"); - } - } - *target = value; - return C_OK; -} - - // ROMAN: Copied from the DISABLED part below int getLongLongFromObject(robj *o, long long *target) { long long value; diff --git a/src/redis/util.c b/src/redis/util.c index 8669d42fe..814244bc7 100644 --- a/src/redis/util.c +++ b/src/redis/util.c @@ -513,27 +513,6 @@ int string2ld(const char *s, size_t slen, long double *dp) { return 1; } -/* Convert a string into a double. Returns 1 if the string could be parsed - * into a (non-overflowing) double, 0 otherwise. The value will be set to - * the parsed value when appropriate. - * - * Note that this function demands that the string strictly represents - * a double: no spaces or other characters before or after the string - * representing the number are accepted. */ -int string2d(const char *s, size_t slen, double *dp) { - errno = 0; - char *eptr; - *dp = strtod(s, &eptr); - if (slen == 0 || - isspace(((const char*)s)[0]) || - (size_t)(eptr-(char*)s) != slen || - (errno == ERANGE && - (*dp == HUGE_VAL || *dp == -HUGE_VAL || *dp == 0)) || - isnan(*dp)) - return 0; - return 1; -} - /* Convert a double to a string representation. Returns the number of bytes * required. The representation should always be parsable by strtod(3). * This function does not support human-friendly formatting like ld2string diff --git a/src/redis/util.h b/src/redis/util.h index fd91ab5bd..56c482d32 100644 --- a/src/redis/util.h +++ b/src/redis/util.h @@ -60,7 +60,6 @@ int string2ll(const char *s, size_t slen, long long *value); int string2ull(const char *s, unsigned long long *value); int string2l(const char *s, size_t slen, long *value); int string2ld(const char *s, size_t slen, long double *dp); -int string2d(const char *s, size_t slen, double *dp); int d2string(char *buf, size_t len, double value); int ld2string(char *buf, size_t len, long double value, ld2string_mode mode); diff --git a/src/server/test_utils.cc b/src/server/test_utils.cc index f9386b50d..ba55e2b3f 100644 --- a/src/server/test_utils.cc +++ b/src/server/test_utils.cc @@ -277,14 +277,21 @@ CmdArgVec BaseFamilyTest::TestConnWrapper::Args(ArgSlice list) { CHECK_NE(0u, list.size()); CmdArgVec res; + string* str = new string; + + // I compact all the arguments together on purpose. + // This way I check that arguments handling works well without c-string endings. + for (auto v : list) { + str->append(v); + } + tmp_str_vec_.emplace_back(str); + size_t offset = 0; for (auto v : list) { if (v.empty()) { res.push_back(MutableSlice{}); } else { - tmp_str_vec_.emplace_back(new string{v}); - auto& s = *tmp_str_vec_.back(); - - res.emplace_back(s.data(), s.size()); + res.emplace_back(str->data() + offset, v.size()); + offset += v.size(); } } diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index 0c7e55324..132b27fef 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -11,7 +11,7 @@ extern "C" { #include "redis/zset.h" } -#include +#include #include "base/logging.h" #include "base/stl_util.h" @@ -23,7 +23,6 @@ extern "C" { namespace dfly { -using namespace double_conversion; using namespace std; using namespace facade; using absl::SimpleAtoi; @@ -96,7 +95,7 @@ OpResult FindZEntry(const ZParams& zparams, const OpArgs& op_args try { add_res = db_slice.AddOrFind(op_args.db_ind, key); - } catch(bad_alloc&) { + } catch (bad_alloc&) { return OpStatus::OUT_OF_MEMORY; } @@ -541,7 +540,9 @@ bool ParseScore(string_view src, double* score) { } else if (src == "+inf") { *score = HUGE_VAL; } else { - return string2d(src.data(), src.size(), score); + absl::from_chars_result result = absl::from_chars(src.data(), src.end(), *score); + if (int(result.ec) != 0 || result.ptr != src.end() || isnan(*score)) + return false; } return true; }; diff --git a/src/server/zset_family_test.cc b/src/server/zset_family_test.cc index 060e6366a..cbdcf7890 100644 --- a/src/server/zset_family_test.cc +++ b/src/server/zset_family_test.cc @@ -243,4 +243,9 @@ TEST_F(ZSetFamilyTest, ZInterStore) { EXPECT_THAT(resp.GetVec(), ElementsAre("b", "4")); } +TEST_F(ZSetFamilyTest, ZAddBug148) { + auto resp = Run({"zadd", "key", "1", "9fe9f1eb"}); + EXPECT_THAT(resp, IntArg(1)); +} + } // namespace dfly