fix: stack overflow in hincrbyfloat (#5835)

* fix: stack overflow in hincrbyfloat

* refactor: address comments

* fix: process errors more accurately for incrbyfloat

Author: BorysTheDev

src/facade/error.h

@@ -38,6 +38,7 @@ inline constexpr char kUndeclaredKeyErr[] = "script tried accessing undeclared k
 inline constexpr char kInvalidDumpValueErr[] = "DUMP payload version or checksum are wrong";
 inline constexpr char kInvalidJsonPathErr[] = "invalid JSON path";
 inline constexpr char kJsonParseError[] = "failed to parse JSON";
+inline constexpr char kNanOrInfDuringIncr[] = "increment would produce NaN or Infinity";
 inline constexpr char kCrossSlotError[] = "-CROSSSLOT Keys in request don't hash to the same slot";
 
 inline constexpr char kSyntaxErrType[] = "syntax_error";

src/facade/op_status.cc

@@ -40,6 +40,8 @@ std::string_view StatusToMsg(OpStatus status) {
       return kInvalidJsonPathErr;
     case OpStatus::INVALID_JSON:
       return kJsonParseError;
+    case OpStatus::NAN_OR_INF_DURING_INCR:
+      return kNanOrInfDuringIncr;
     default:
       LOG(ERROR) << "Unsupported status " << status;
       return "Internal error";

src/facade/op_status.h

@@ -33,7 +33,8 @@ enum class OpStatus : uint16_t {
   MEMBER_NOTFOUND,
   INVALID_JSON_PATH,
   INVALID_JSON,
-  IO_ERROR
+  IO_ERROR,
+  NAN_OR_INF_DURING_INCR,
 };
 
 class OpResultBase {

src/redis/util.c

@@ -207,33 +207,3 @@ int string2ll(const char *s, size_t slen, long long *value) {
     }
     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 string2ld(const char *s, size_t slen, long double *dp) {
-    char buf[MAX_LONG_DOUBLE_CHARS];
-    long double value;
-    char *eptr;
-
-    if (slen == 0 || slen >= sizeof(buf)) return 0;
-    memcpy(buf,s,slen);
-    buf[slen] = '\0';
-
-    errno = 0;
-    value = strtold(buf, &eptr);
-    if (isspace(buf[0]) || eptr[0] != '\0' ||
-        (size_t)(eptr-buf) != slen ||
-        (errno == ERANGE &&
-            (value == HUGE_VAL || value == -HUGE_VAL || value == 0)) ||
-        errno == EINVAL ||
-        isnan(value))
-        return 0;
-
-    if (dp) *dp = value;
-    return 1;
-}

src/redis/util.h

@@ -47,7 +47,6 @@
 
 int ll2string(char *s, size_t len, long long value);
 int string2ll(const char *s, size_t slen, long long *value);
-int string2ld(const char *s, size_t slen, long double *dp);
 
 #define LOG_MAX_LEN    1024 /* Default maximum length of syslog messages.*/
 

src/server/dfly_main.cc

@@ -102,10 +102,10 @@ namespace {
 // need additional 8-16 bytes for their internal structures, thus over reserving additional
 // memory pages if using round sizes.
 #ifdef NDEBUG
-constexpr size_t kFiberDefaultStackSize = 36_KB - 16;
+constexpr size_t kFiberDefaultStackSize = 32_KB - 16;
 #else
-// Increase stack size for debug builds.
-constexpr size_t kFiberDefaultStackSize = 40_KB - 16;
+// We leave the same stack size for debug mode, to find out the possible stack overflow bugs.
+constexpr size_t kFiberDefaultStackSize = 32_KB - 16;
 #endif
 
 enum class TermColor { kDefault, kRed, kGreen, kYellow };

src/server/hset_family.cc

@@ -131,16 +131,16 @@ size_t HMapLength(const DbContext& db_cntx, const CompactObj& co) {
 OpStatus IncrementValue(optional<string_view> prev_val, IncrByParam* param) {
   if (holds_alternative<double>(*param)) {
     double incr = get<double>(*param);
-    long double value = 0;
+    double value = 0;
 
     if (prev_val) {
-      if (!string2ld(prev_val->data(), prev_val->size(), &value)) {
+      if (!ParseDouble(*prev_val, &value)) {
         return OpStatus::INVALID_VALUE;
       }
     }
     value += incr;
     if (isnan(value) || isinf(value)) {
-      return OpStatus::INVALID_FLOAT;
+      return OpStatus::NAN_OR_INF_DURING_INCR;
     }
 
     param->emplace<double>(value);

src/server/hset_family_test.cc

@@ -197,6 +197,20 @@ TEST_F(HSetFamilyTest, HincrbyFloat) {
   }
 }
 
+TEST_F(HSetFamilyTest, HincrbyFloatCornerCases) {
+  Run({"hset", "k", "mhv", "-1.8E+308", "phv", "1.8E+308", "nd", "-+-inf", "+inf", "+inf", "nan",
+       "nan", "-inf", "-inf"});
+  // we don't support long doubles, so in all next cases we should return errors
+  EXPECT_THAT(Run({"hincrbyfloat", "k", "mhv", "-1"}), ErrArg("ERR hash value is not a float"));
+  EXPECT_THAT(Run({"hincrbyfloat", "k", "phv", "1"}), ErrArg("ERR hash value is not a float"));
+  EXPECT_THAT(Run({"hincrbyfloat", "k", "nd", "1"}), ErrArg("ERR hash value is not a float"));
+  EXPECT_THAT(Run({"hincrbyfloat", "k", "+inf", "1"}),
+              ErrArg("increment would produce NaN or Infinity"));
+  EXPECT_THAT(Run({"hincrbyfloat", "k", "nan", "1"}), ErrArg("ERR hash value is not a float"));
+  EXPECT_THAT(Run({"hincrbyfloat", "k", "-inf", "1"}),
+              ErrArg("increment would produce NaN or Infinity"));
+}
+
 TEST_F(HSetFamilyTest, HRandFloat) {
   Run({"HSET", "k", "1", "2"});
 

src/server/string_family.cc

@@ -269,7 +269,7 @@ OpResult<double> OpIncrFloat(const OpArgs& op_args, string_view key, double val)
   base += val;
 
   if (isnan(base) || isinf(base)) {
-    return OpStatus::INVALID_FLOAT;
+    return OpStatus::NAN_OR_INF_DURING_INCR;
   }
 
   char* str = RedisReplyBuilder::FormatDouble(base, buf, sizeof(buf));

src/server/string_family_test.cc

@@ -519,6 +519,10 @@ TEST_F(StringFamilyTest, IncrByFloat) {
   auto resp = Run({"INCRBYFLOAT", "nonum", "1.0"});
   EXPECT_THAT(resp, ErrArg("not a valid float"));
 
+  Run({"SET", "inf", "+inf"});
+  resp = Run({"INCRBYFLOAT", "inf", "1.0"});
+  EXPECT_THAT(resp, ErrArg("increment would produce NaN or Infinity"));
+
   Run({"SET", "nonum", "11 "});
   resp = Run({"INCRBYFLOAT", "nonum", "1.0"});
   EXPECT_THAT(resp, ErrArg("not a valid float"));