Skip to content

Commit 884bf46

Browse files
netliomax25-codeapolukhin
authored andcommitted
fix redis: use FromString for double reply values
1. `Parse(..., To<double>)` converts floating-point reply values (`ZSCORE`, `HINCRBYFLOAT`, `ZADD ... INCR`) with `std::stod`, which respects the process `LC_NUMERIC` and keeps only the valid numeric prefix of the string. 2. under a comma-decimal locale `"3.14"` parses as `3.0`, and a reply like `"3.14x"` parses as `3.14` with the trailing bytes silently dropped, so a compromised or MITM'd server can return a corrupted value. Switched to `utils::FromString<double>`, matching the geo parser at line 58 and the score parser at line 166 in this file: it is locale-independent and rejects trailing junk, while numbers, scientific notation, and `inf`/`-inf` still parse the same. Added a regression test for the trailing-junk case along with valid and infinite values. --- Pull Request resolved: #1297 Co-authored-by: antoshkka <antoshkka@userver.tech> commit_hash:e5b7997828ad3c0a5d02d03bf9a62d907405f3bc
1 parent 5392e2a commit 884bf46

3 files changed

Lines changed: 47 additions & 1 deletion

File tree

.mapping.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4339,6 +4339,7 @@
43394339
"redis/src/storages/redis/json_cluster_redistest.cpp":"taxi/uservices/userver/redis/src/storages/redis/json_cluster_redistest.cpp",
43404340
"redis/src/storages/redis/key_type.cpp":"taxi/uservices/userver/redis/src/storages/redis/key_type.cpp",
43414341
"redis/src/storages/redis/parse_reply.cpp":"taxi/uservices/userver/redis/src/storages/redis/parse_reply.cpp",
4342+
"redis/src/storages/redis/parse_reply_test.cpp":"taxi/uservices/userver/redis/src/storages/redis/parse_reply_test.cpp",
43424343
"redis/src/storages/redis/pttl_reply.cpp":"taxi/uservices/userver/redis/src/storages/redis/pttl_reply.cpp",
43434344
"redis/src/storages/redis/pubsub_redistest.cpp":"taxi/uservices/userver/redis/src/storages/redis/pubsub_redistest.cpp",
43444345
"redis/src/storages/redis/pubsub_redistest.hpp":"taxi/uservices/userver/redis/src/storages/redis/pubsub_redistest.hpp",

redis/src/storages/redis/parse_reply.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ std::string Parse(ReplyData&& reply_data, const std::string& request_description
263263
double Parse(ReplyData&& reply_data, const std::string& request_description, To<double>) {
264264
reply_data.ExpectString(request_description);
265265
try {
266-
return std::stod(reply_data.GetString());
266+
return utils::FromString<double>(reply_data.GetString());
267267
} catch (const std::exception& ex) {
268268
throw ParseReplyException(
269269
"Can't parse value from reply to '" + request_description + "' request (" + reply_data.ToDebugString() +
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
#include <userver/storages/redis/parse_reply.hpp>
2+
3+
#include <cmath>
4+
5+
#include <gtest/gtest.h>
6+
7+
#include <userver/storages/redis/exception.hpp>
8+
#include <userver/storages/redis/reply.hpp>
9+
10+
USERVER_NAMESPACE_BEGIN
11+
12+
namespace {
13+
14+
double ParseDouble(std::string value) {
15+
return storages::redis::Parse(
16+
storages::redis::ReplyData{std::move(value)},
17+
"test_request",
18+
storages::redis::To<double>{}
19+
);
20+
}
21+
22+
} // namespace
23+
24+
TEST(ParseReply, DoubleValid) {
25+
EXPECT_DOUBLE_EQ(ParseDouble("3.14"), 3.14);
26+
EXPECT_DOUBLE_EQ(ParseDouble("-2.5"), -2.5);
27+
EXPECT_DOUBLE_EQ(ParseDouble("1e3"), 1000.0);
28+
}
29+
30+
TEST(ParseReply, DoubleInfinity) {
31+
// ZSCORE and friends report infinite scores as "inf"/"-inf".
32+
EXPECT_TRUE(std::isinf(ParseDouble("inf")));
33+
EXPECT_TRUE(std::isinf(ParseDouble("-inf")));
34+
}
35+
36+
TEST(ParseReply, DoubleTrailingJunk) {
37+
// A compromised, misbehaving, or MITM'd server can return a bulk string with
38+
// a valid numeric prefix followed by junk. std::stod silently accepted the
39+
// prefix and dropped the rest; the strict parser rejects the whole reply.
40+
EXPECT_THROW(ParseDouble("3.14garbage"), storages::redis::ParseReplyException);
41+
EXPECT_THROW(ParseDouble("nonsense"), storages::redis::ParseReplyException);
42+
EXPECT_THROW(ParseDouble(""), storages::redis::ParseReplyException);
43+
}
44+
45+
USERVER_NAMESPACE_END

0 commit comments

Comments
 (0)