Tags: facebook/mcrouter
Tags
Hoist forEach option names into static const std::string to avoid per… …-call temporaries Reviewed By: DenisYaroshevskiy Differential Revision: D109159609 fbshipit-source-id: 7257538ebda60d7b9ec3abcfb5d29fa2e6fd5e6a
Capture precomputed backup file name by move instead of copying name/… …sourcePrefix Reviewed By: jpporto Differential Revision: D109159277 fbshipit-source-id: cc28512b422af51ac7dca7cb019c51de3c47367a
Stop mutating dynamic object keys in config macros
Summary:
## Goal.
Replace UB behavior of `const_cast`-ing elements which may be `const`-storage allocated with a proper drain semantics.
## Context.
The current implementation of mcrouter performs operations of the form:
```
for (auto& item : object.items()) {
auto& key = const_cast<folly::dynamic&>(item.first);
auto& value = item.second;
output.insert(std::move(key), std::move(value));
}
```
As explained in D108334544 this can generally be a UB and cause bad outcomes since the compiler is free to optimize away the const storage such that any mutation of it can lead to an undefined behavior. In production we have found clusters of cores and ASAN failures which show invalid memory being accessed within the keys of objects which implies that this may have production impact. Even if that is not the case, avoiding UB is a strict improvement and hardening.
## This diff
`const_cast` is used as a crutch since there are no extraction semantics supported within `folly::object::dictionary`. The diff below adds such semantics and allows safe, zero-copy move out of one container into another. This diff introduces no behavioral changes at all, but it does improve safety.
## Cores and more context
Direct ASAN core analysis of ASAN-instrumented `thatch_vm` tasks shows crashes while mcrouter is preprocessing config macros on the config thread. Cores `lzunp38fulwr6r2k` and `r93hc7pfo7ex3tcs` abort while destroying a `folly::dynamic::object` from `facebook::memcache::ConfigPreprocessor::expandMacros`; core `279y5ckx34qb9q67` separately reaches a bad/end F14 iterator dereference in `ConfigPreprocessor::Context::expandRawArg` through `BuiltIns::ifMacro`. Production core stack paste: https://www.internalfb.com/intern/paste/P2374616358/
Inline production stack summary:
```
operator delete
folly::f14::detail::F14Table<folly::dynamic, folly::dynamic>::reset
folly::dynamic::destroy
folly::dynamic::ObjectMaker::~ObjectMaker
Reviewed By: snarkmaster
Differential Revision: D108301752
fbshipit-source-id: aad384111cd8c80f01788c6a9692f83fa993627b
add SetDistributionTargetRoute to stamp DL target from config
Summary:
New route handle SetDistributionTargetRoute stamps the distributionTargetRegion fiber-local (inside runWithLocals) and delegates to its child, so the ucache pool's auto-wrapped DistributionRoute broadcasts the write to DL. C++ half of moving the ads adm:* global-replication prefixes off legacy fabric broadcast onto DL.
route(): (a) target already stamped (e.g. RootRoute's directed stamp) -> delegate; (b) non-empty routing prefix -> delegate (don't convert directed into broadcast); (c) empty prefix -> stamp target_region ("" = broadcast) and route child in-scope. Lives in routes/ (no FOLLY_ATTR_WEAK), registered in buildRouteMap().
Also bumps mcrouter ClientVersion to 182 so the config can gate DL on mc-client-version >= 182. Land before the configerator change.
Reviewed By: stuclar
Differential Revision: D107156224
fbshipit-source-id: 8db84ba22df57827b897c7b6c77c767f7a9b5de6
closeNow() before socket_.reset() to fix Fizz handshake UAF Summary: fizz::client::AsyncFizzClient stores its ConnectCallback as a raw pointer and arms a handshake-timeout timer when connect() is called. On the inline-Fizz outbound path in mcrouter, that pointer is the AsyncMcClientImpl. If the underlying McFizzClient transport outlives AsyncMcClientImpl with the timer still armed, deliverHandshakeError dereferences a freed ConnectCallback and crashes. The race window scales with the configured connect/handshake timeout. Two paths in AsyncMcClientImpl destroy the transport via socket_.reset() without first calling socket_->closeNow(): - connectErr() - processShutdown() closeNow() runs deliverAllErrors -> deliverHandshakeError synchronously, which cancels the Fizz handshake-timeout timer, clears the raw ConnectCallback pointer, and delivers a terminal connectErr(). After closeNow(), even if a DestructorGuard keeps the transport alive past socket_.reset(), the dangling-pointer hazard is gone. This matches the existing pattern in ~AsyncMcClientImpl(). Both reset()-without-closeNow() sites now follow it. Out of scope: the async SSL handshake-offload path (already anchored via selfPtr_.lock()), plaintext / non-Fizz, and inbound (server-side) Fizz handshakes (already fixed separately). Reviewed By: knekritz Differential Revision: D105440074 fbshipit-source-id: 4be476feab9041e900f93db690a32d3ac7d8e744
Make carbon-generated Messages headers self-contained Summary: X-link: facebookresearch/DCPerf#631 `Messages-inl.h` is not "self-contained", meaning a .cpp file that only includes it will fail to compile. This blocks some build time optimizations like C++20 header units. -inl.h are discouraged for that reason (https://www.internalfb.com/wiki/Cpp/CppCodingConventions#avoid-using-inl-h-files), and the proper way is to just list all templates directly in the including file. But we can make a narrow change instead to avoid large scale code movements. Introduce `Messages-decl.h` (declarations only) so that both `Messages.h` and `Messages-inl.h` are self-contained: - `Messages-decl.h`: declarations, type traits, free function forward declarations. Identical to the current `Messages.h` without the last `#include "Messages-inl.h"` line. - `Messages-inl.h`: adds `#pragma once` and `#include "Messages-decl.h"` at top, otherwise unchanged - `Messages.h`: thin wrapper that includes `-decl.h` and `-inl.h` Reviewed By: IvanTopolcic Differential Revision: D106002540 fbshipit-source-id: 3153f0adb2325293e5b50159aca8609c7fa0dd17
mcrouter: log lease_token to mcrouter_requests Scuba dataset Summary: Adds a `lease_token` int column to the `mcrouter_requests` Scuba dataset for `McLeaseGetReply` and `McLeaseSetRequest`. The column is omitted for non-lease ops, so a token value of `0` is distinguishable from absence. Only `FbAdditionalProxyRequestLogger` reads the token via the new `getLeaseTokenIfExist` SFINAE helper; `UcacheLogger` and `TaoLogger` pass `std::nullopt` since their wrapper types never carry leases. `if constexpr` makes the helper a no-op for non-lease types. S575723 follow-up: lease tokens were not logged anywhere, making the KCB bug hard to debug from client traces. Reviewed By: stuclar Differential Revision: D105033460 fbshipit-source-id: c7d586ce782a64eb54fab0d7ab9bd6b5558a4867
fbcode/mcrouter/routes/McRouteHandleProvider-inl.h Reviewed By: MatzeB Differential Revision: D104066553 fbshipit-source-id: 0a37e97d59a1f7287ef32ab54a90afe0b1569239
Enable Pyrefly in fbcode/mcrouter Summary: Automated migration to enable Pyrefly type checking for `fbcode/mcrouter`. - Added `python.set_pyrefly(True)` to PACKAGE file - Suppressed pre-existing type errors Pyrefly is Meta's next-generation Python type checker, replacing Pyre. If you encounter issues, you can revert the PACKAGE change by removing the `python.set_pyrefly(True)` line. #pyreupgrade Differential Revision: D103425534 fbshipit-source-id: dbea23f732c5b6d3a9e8e2f0de0c75a8803b6ad4
Make ExternalCarbonConnectionImpl fiber stack size a gflag (default 6… …4 KiB) Summary: Human Darrien - For the mcrouter folks, let us know if this is a reasonable change to make. We are getting fiber overflows in our tests as of late since D100058297 in our tests. We're hoping to provide an override in the least invasive way so we can have these tests run again. Let us know if this is reasonable 🙂↕️ *** The `fbjava/privacy/zone:zone_library_test` integration tests fail in `mode/dev` with `folly::fibers Fiber stack overflow detected`. Root cause: `mcrouter/lib/carbon/connection/ExternalCarbonConnectionImpl.cpp`'s `ThreadInfo` constructs its `FiberManager` with the default `Options` (16 KiB stack), which becomes 32 KiB in dev after the `stackSizeMultiplier=2` is applied. The `AsyncMcClient::create` path running on that fiber overflows the 32 KiB stack in unoptimized builds where frames are larger. Added a `cacheclient_external_fiber_stack_size` gflag defaulting to 65536 bytes (64 KiB), and wired it into the `FiberManager::Options.stackSize` used by `ThreadInfo`. The new default gives 64 KiB × 2 = 128 KiB in dev (and 64 KiB in opt), enough headroom for `AsyncMcClient::create` while still being reasonable for a per-fiber allocation. Operators can tune this further at runtime if needed. Differential Revision: D101644600 fbshipit-source-id: b75d788c6339a76265de634da7e23af83cb70d30
PreviousNext