Skip to content

Commit

Permalink
memcpy empty string issue
Browse files Browse the repository at this point in the history
Summary:
memcpying from a nullptr is a UB.

UBSan caught it on one of the rollout tests

This unfortunately has an overhead of an extra if but given that most strings are bigger than 8 chars, hopefully not a big one.

Reviewed By: udippant, elalfer

Differential Revision: D51754836

fbshipit-source-id: 50069195a3ac8121fc42f9310d43c49c17831917
  • Loading branch information
DenisYaroshevskiy authored and facebook-github-bot committed Dec 1, 2023
1 parent c282591 commit db168f7
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
6 changes: 4 additions & 2 deletions mcrouter/lib/fbi/cpp/LowerBoundPrefixMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@ class SmallPrefix {
data_ = folly::Endian::swap(data_);
return;
}
std::memcpy(&data_, s.data(), s.size());
data_ = folly::Endian::swap(data_);
if (FOLLY_LIKELY(s.data() != nullptr)) {
std::memcpy(&data_, s.data(), s.size());
data_ = folly::Endian::swap(data_);
}
}

// default operator== and operator<=> are ok here
Expand Down
5 changes: 5 additions & 0 deletions mcrouter/lib/fbi/cpp/test/LowerBoundPrefixMapTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,5 +205,10 @@ TEST(LowerBoundPrefixMapTest, EmptyMap) {
ASSERT_TRUE(lbMap.empty());
}

TEST(LowerBoundPrefixMapTest, NullKey) {
LowerBoundPrefixMap<int> lbMap;
ASSERT_EQ(lbMap.findPrefix(std::string_view{}), lbMap.end());
}

} // namespace
} // namespace facebook::memcache

0 comments on commit db168f7

Please sign in to comment.