fix BigValueRoute sscanf over-read on non-NUL-terminated reply

Summary:
Root cause: BigValueRoute<RouterInfo>::ChunksInfo(folly::StringPiece replyValue) parsed the chunks-info header by calling sscanf(replyValue.data(), "%u-%u-%lu%n", ...). The StringPiece is a (ptr, len) view over a coalesced IOBuf reply payload and is NOT NUL-terminated. glibc sscanf computes the input C-string length up front by scanning for a NUL with rawmemchr (via _IO_str_init_static_internal called with size=-1), BEFORE any parsing happens. When the byte after the payload is not a NUL, that scan reads past the buffer; if the payload abuts an unmapped page the read SIGSEGVs. The existing charsRead == replyValue.size() guard cannot prevent it because it runs only after the faulting sscanf call returns.

Evidence: confirmed in two production CachiusServer coredumps on mcrpxy-web* threads. In the non-marker core the rawmemchr fault register was rdi=0x7f8744dfffe0, exactly 0x20 below the page boundary 0x7f8744e00000, with rsi=0 (scanning for NUL) - i.e. a length-scan running off the end of a mapped page. Both cores had a valid v1 ChunksInfo input ("1-2-7419256", "1-2-3745222203") followed by adjacent garbage, so the crash requires no malformed or attacker-controlled data. This accounts for ~480 SIGSEGV/week (~19% of CachiusServer crashes); it is steady and version-independent. (It is distinct from the larger CachiusCPUPool SIGABRT population, which is unrelated to mcrouter.)

Fix: parse strictly within the StringPiece bounds. Extracted a bounded detail::parseChunksInfo() helper using folly::split('-', ...) (exact 3 fields) + folly::tryTo<uint32_t/uint32_t/uint64_t>, which never reads past replyValue. Semantics are preserved (and slightly stricter on malformed input: leading '+', overflow, and negative fields are now rejected - none reachable from the toStringType() producer, which emits pure "{}-{}-{}" digits). numChunks_/suffix_ are now zero-initialized.

Bumps ClientVersion 182 -> 183.

Reviewed By: disylh

Differential Revision: D107896217

fbshipit-source-id: 77e80ddc660d66e46b0f0bbc66ec2028aec261cc

Author: Lenar Fatikhov

mcrouter/routes/BigValueRoute-inl.h

@@ -12,7 +12,9 @@
 
 #include <fmt/format.h>
 
+#include <folly/Conv.h>
 #include <folly/Range.h>
+#include <folly/String.h>
 #include <folly/fibers/FiberManager.h>
 #include <folly/fibers/WhenN.h>
 #include <folly/io/Cursor.h>
@@ -52,6 +54,30 @@ uint64_t hashBigValue(const folly::IOBuf& value);
 
 BigValueRouteOptions parseBigValueRouteSettings(const folly::dynamic& json);
 
+// Parses a chunks-info string of the form "version-numChunks-suffix".
+// replyValue is not NUL-terminated (it views into an IOBuf payload), so it must
+// never be passed to sscanf, which scans for a NUL and reads past the buffer.
+inline bool parseChunksInfo(
+    folly::StringPiece replyValue,
+    uint32_t& version,
+    uint32_t& numChunks,
+    uint64_t& suffix) {
+  folly::StringPiece versionStr, numChunksStr, suffixStr;
+  if (!folly::split('-', replyValue, versionStr, numChunksStr, suffixStr)) {
+    return false;
+  }
+  auto v = folly::tryTo<uint32_t>(versionStr);
+  auto n = folly::tryTo<uint32_t>(numChunksStr);
+  auto s = folly::tryTo<uint64_t>(suffixStr);
+  if (!v || !n || !s) {
+    return false;
+  }
+  version = *v;
+  numChunks = *n;
+  suffix = *s;
+  return true;
+}
+
 } // namespace detail
 
 template <class RouterInfo>
@@ -383,20 +409,9 @@ McLeaseGetReply BigValueRoute<RouterInfo>::doLeaseGetRoute(
 
 template <class RouterInfo>
 BigValueRoute<RouterInfo>::ChunksInfo::ChunksInfo(folly::StringPiece replyValue)
-    : infoVersion_(1), valid_(true) {
-  // Verify that replyValue is of the form version-numChunks-suffix,
-  // where version, numChunks and suffix should be numeric
-  uint32_t version;
-  int charsRead;
-  valid_ &=
-      (sscanf(
-           replyValue.data(),
-           "%u-%u-%lu%n",
-           &version,
-           &numChunks_,
-           &suffix_,
-           &charsRead) == 3);
-  valid_ &= (static_cast<size_t>(charsRead) == replyValue.size());
+    : infoVersion_(1), numChunks_(0), suffix_(0), valid_(true) {
+  uint32_t version = 0;
+  valid_ &= detail::parseChunksInfo(replyValue, version, numChunks_, suffix_);
   valid_ &= (version == infoVersion_);
 }
 

mcrouter/routes/test/BigValueRouteTest.cpp

@@ -26,3 +26,31 @@ TEST(BigValueRouteTest, bigvalue) {
   facebook::memcache::mcrouter::testBigvalue<
       facebook::memcache::MemcacheRouterInfo>();
 }
+
+TEST(BigValueRouteTest, chunksInfoParsing) {
+  namespace mc = facebook::memcache::mcrouter;
+  uint32_t version = 0, numChunks = 0;
+  uint64_t suffix = 0;
+
+  EXPECT_TRUE(
+      mc::detail::parseChunksInfo("1-2-7419256", version, numChunks, suffix));
+  EXPECT_EQ(1, version);
+  EXPECT_EQ(2, numChunks);
+  EXPECT_EQ(7419256, suffix);
+
+  EXPECT_FALSE(mc::detail::parseChunksInfo("1-2", version, numChunks, suffix));
+  EXPECT_FALSE(
+      mc::detail::parseChunksInfo("1-2-3-4", version, numChunks, suffix));
+  EXPECT_FALSE(
+      mc::detail::parseChunksInfo("1-2-x", version, numChunks, suffix));
+  EXPECT_FALSE(mc::detail::parseChunksInfo("", version, numChunks, suffix));
+
+  // Regression: must stay within the StringPiece bounds and never read trailing
+  // bytes. The crash was sscanf scanning a non-NUL-terminated buffer past its
+  // end.
+  folly::StringPiece backing("1-2-3456789");
+  EXPECT_TRUE(
+      mc::detail::parseChunksInfo(
+          backing.subpiece(0, 7), version, numChunks, suffix));
+  EXPECT_EQ(345, suffix);
+}