diff --git a/redis/src/storages/redis/parse_reply.cpp b/redis/src/storages/redis/parse_reply.cpp index 6afc9c34a46c..cb7291877a0d 100644 --- a/redis/src/storages/redis/parse_reply.cpp +++ b/redis/src/storages/redis/parse_reply.cpp @@ -198,6 +198,14 @@ std::vector ParseReplyDataArray(ReplyData&& array_data, [[maybe_unused "Can't parse value from reply to '" + request_description + ", additional_info item is empty array" ); } + if (!additional_infos[0].IsString()) { + throw ParseReplyException( + "Can't parse value from reply to '" + request_description + "', expected " + + ReplyData::TypeToString(ReplyData::Type::kString) + + " as the first additional_info element, got type: " + additional_infos[0].GetTypeString() + + " elem=" + additional_infos[0].ToDebugString() + ); + } geo_point.member = std::move(additional_infos[0].GetString()); for (size_t i = 1; i < additional_infos.size(); ++i) { diff --git a/redis/src/storages/redis/parse_reply_test.cpp b/redis/src/storages/redis/parse_reply_test.cpp new file mode 100644 index 000000000000..c9e2b41371e4 --- /dev/null +++ b/redis/src/storages/redis/parse_reply_test.cpp @@ -0,0 +1,59 @@ +#include + +#include + +#include +#include +#include + +USERVER_NAMESPACE_BEGIN + +// A GEORADIUS/GEOSEARCH reply with WITHDIST/WITHCOORD/WITHHASH is an array of +// per-member arrays whose first element is the member name (a bulk string). A +// malicious, compromised or MITM'd Redis server can return a non-string there. +// The first element used to be read with GetString() without a type check, so +// the parser dereferenced the null returned by std::get_if in a release build. +TEST(ParseReply, GeoPointFirstElementNotString) { + using storages::redis::ReplyData; + + ReplyData::Array member_info; + member_info.emplace_back(ReplyData(42)); // member name, but an int + member_info.emplace_back(ReplyData(std::string{"1.5"})); // distance + + ReplyData::Array top; + top.emplace_back(ReplyData(std::move(member_info))); + + ReplyData reply{std::move(top)}; + + EXPECT_THROW( + storages::redis::ParseReplyDataArray( + std::move(reply), + "GEORADIUS", + storages::redis::To>{} + ), + storages::redis::ParseReplyException + ); +} + +TEST(ParseReply, GeoPointFirstElementString) { + using storages::redis::ReplyData; + + ReplyData::Array member_info; + member_info.emplace_back(ReplyData(std::string{"Palermo"})); + member_info.emplace_back(ReplyData(std::string{"190.4424"})); + + ReplyData::Array top; + top.emplace_back(ReplyData(std::move(member_info))); + + ReplyData reply{std::move(top)}; + + auto result = storages::redis::ParseReplyDataArray( + std::move(reply), + "GEORADIUS", + storages::redis::To>{} + ); + ASSERT_EQ(result.size(), 1); + EXPECT_EQ(result[0].member, "Palermo"); +} + +USERVER_NAMESPACE_END