Repository navigation
[TransferEngine] Guard MC_TCP_SLICE_SIZE parsing against std::stoull throwing - #2641
Conversation
…throwing getChunkSize() reads the MC_TCP_SLICE_SIZE env var and parses it with std::stoull, but only handles the case where stoull succeeds and returns a positive value (otherwise it falls back to the 64KB default). A value that std::stoull cannot parse - a non-numeric string, an empty string, or an out-of-range number - makes std::stoull throw std::invalid_argument / std::out_of_range. The parse runs inside the static-local initializer, and neither getChunkSize() nor the transfer paths that first call it (TcpTransport read/write) catch the exception, so a typo in the env var aborts the transfer instead of warning and using the default. Wrap the parse in try/catch and route both the throwing case and a non-positive value through the same warning-and-default path, mirroring the existing MC_MS_AUTO_DISC guard (kvcache-ai#2629) and the env parsing in config.cpp (MC_PKEY_INDEX / MC_IB_TC / MC_IB_SL), so any invalid MC_TCP_SLICE_SIZE value degrades gracefully. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request wraps the parsing of the MC_TCP_SLICE_SIZE environment variable in a try-catch block to prevent exceptions from propagating and aborting the transfer. The reviewer identified an issue where std::stoull can parse negative inputs (such as "-1") into extremely large unsigned values, which would bypass the v > 0 check and potentially cause out-of-memory errors. The reviewer suggested using std::stoll to safely parse and validate the input instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| try { | ||
| size_t v = std::stoull(env); | ||
| if (v > 0) return v; |
There was a problem hiding this comment.
Using std::stoull to parse the environment variable can lead to unexpected behavior if the input is negative (e.g., "-1"). Under the hood, std::stoull (via std::strtoull) allows a leading minus sign and negates the resulting unsigned value, converting "-1" into 18446744073709551615 (ULLONG_MAX). This huge value passes the v > 0 check and is returned as the chunk size, which can cause extremely large memory allocations and potential out-of-memory crashes later in the transport code.
To prevent this, you can parse the value using std::stoll (which returns a signed long long). This way, negative values are correctly parsed as negative numbers, fail the v > 0 check, and safely fall back to the default value.
| try { | |
| size_t v = std::stoull(env); | |
| if (v > 0) return v; | |
| try { | |
| long long v = std::stoll(env); | |
| if (v > 0) return static_cast<size_t>(v); |
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…throwing (kvcache-ai#2641) * [TransferEngine] Guard MC_TCP_SLICE_SIZE parsing against std::stoull throwing getChunkSize() reads the MC_TCP_SLICE_SIZE env var and parses it with std::stoull, but only handles the case where stoull succeeds and returns a positive value (otherwise it falls back to the 64KB default). A value that std::stoull cannot parse - a non-numeric string, an empty string, or an out-of-range number - makes std::stoull throw std::invalid_argument / std::out_of_range. The parse runs inside the static-local initializer, and neither getChunkSize() nor the transfer paths that first call it (TcpTransport read/write) catch the exception, so a typo in the env var aborts the transfer instead of warning and using the default. Wrap the parse in try/catch and route both the throwing case and a non-positive value through the same warning-and-default path, mirroring the existing MC_MS_AUTO_DISC guard (kvcache-ai#2629) and the env parsing in config.cpp (MC_PKEY_INDEX / MC_IB_TC / MC_IB_SL), so any invalid MC_TCP_SLICE_SIZE value degrades gracefully. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * clang-format the new MC_TCP_SLICE_SIZE warning logs Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> --------- Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Problem
getChunkSize()intcp_transport.cppreads theMC_TCP_SLICE_SIZEenv var and parses it withstd::stoull:The lambda only handles the success path (a positive value; otherwise it falls back to the 64KB default). But a value that
std::stoullcannot parse makes it throw:64K/true) ->std::invalid_argumentexport MC_TCP_SLICE_SIZE=, wheregetenvreturns a non-null empty string) ->std::invalid_argumentstd::out_of_rangeThe parse runs inside the static-local initializer, and neither
getChunkSize()nor its callers (theTcpTransportread/write paths at thestd::min(getChunkSize(), ...)call sites) catch the exception. So a misconfigured env var aborts the first TCP transfer instead of warning and using the default, which is what the existingif (v > 0)branch already intends for bad input.Fix
Wrap the parse in try/catch and route both the throwing case and a non-positive value through a warning-and-default path, so any invalid
MC_TCP_SLICE_SIZEvalue degrades gracefully.This mirrors the env-var parsing that already exists in this codebase:
get_auto_discover()guardingMC_MS_AUTO_DISCagainststd::stoithrowing ([Store] Don't abort client init on a malformed MC_MS_AUTO_DISC value #2629)config.cppguardingMC_PKEY_INDEX/MC_IB_TC/MC_IB_SL/MC_MLX5_QP_UDP_SPORTSwith the same try/catch + warning + default shapeVerification
13 lines, single file. I did not build locally (the transfer engine needs an RDMA/CUDA toolchain I don't have set up); the change is verified by the C++ standard semantics of
std::stoull(throwsstd::invalid_argument/std::out_of_rangeon the inputs above) and by matching the established sibling guards listed above, and relies on the existing build/CI jobs to compile-check it. The default-path behavior (64KB) is unchanged for valid and absent values.